init and vault create no longer put the mnemonic into the environment for vault.CreateVault to read back. Each command that may need SB_SECRET_MNEMONIC or SB_UNLOCK_PASSPHRASE reads both once, in its RunE, into locked buffers on the CLI Instance, and unsets them straight away, so the programs it runs, gpg included, do not inherit them. Nothing below the command reads the environment; the buffers are passed down:
vault.CreateVault takes the mnemonic (nil for none) and sets it on the vault it returns.
A Vault carries Mnemonic and UnlockPassphrase, and gives the passphrase to a passphrase unlocker.
Secret.GetValue takes the mnemonic; the PGP, keychain and Secure Enclave unlocker constructors take both. CreatePGPUnlocker sets them on the vault it loads with SetMnemonic and SetUnlockPassphrase, new in VaultInterface.
The README's Environment Variables section has the warning.
Worth knowing:
Unsetting does not hide a value from /proc/PID/environ, which shows the environment the process started with. Comments and README say only what unsetting does.
A passphrase unlocker copied a passphrase it was given with NewBufferFromBytes, which wipes the source. Nothing gave it one before; it now copies without wiping.
Tests give buffers instead of setting the environment, so many now run in parallel.
The integration test's concurrent get readers now run as separate processes: in one process the first unsets the mnemonic for the rest.
vault create uses init's mnemonic prompt instead of its own copy.
No gate compiles the darwin-only files; I checked them by reading only.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/60.
`init` and `vault create` no longer put the mnemonic into the environment for `vault.CreateVault` to read back. Each command that may need `SB_SECRET_MNEMONIC` or `SB_UNLOCK_PASSPHRASE` reads both once, in its `RunE`, into locked buffers on the CLI `Instance`, and unsets them straight away, so the programs it runs, `gpg` included, do not inherit them. Nothing below the command reads the environment; the buffers are passed down:
- `vault.CreateVault` takes the mnemonic (nil for none) and sets it on the vault it returns.
- A `Vault` carries `Mnemonic` and `UnlockPassphrase`, and gives the passphrase to a passphrase unlocker.
- `Secret.GetValue` takes the mnemonic; the PGP, keychain and Secure Enclave unlocker constructors take both. `CreatePGPUnlocker` sets them on the vault it loads with `SetMnemonic` and `SetUnlockPassphrase`, new in `VaultInterface`.
The README's Environment Variables section has the warning.
Worth knowing:
- Unsetting does not hide a value from /proc/PID/environ, which shows the environment the process started with. Comments and README say only what unsetting does.
- A passphrase unlocker copied a passphrase it was given with `NewBufferFromBytes`, which wipes the source. Nothing gave it one before; it now copies without wiping.
- Tests give buffers instead of setting the environment, so many now run in parallel.
- The integration test's concurrent `get` readers now run as separate processes: in one process the first unsets the mnemonic for the rest.
- `vault create` uses `init`'s mnemonic prompt instead of its own copy.
- No gate compiles the darwin-only files; I checked them by reading only.
Model: opus-5-5
Breaks a test now on next. I rebased onto current next, which has the vault name checks from #68. After that, the four vault create cases of TestInvalidVaultNameLeavesStateUnchanged (internal/cli/path_traversal_test.go) fail. The test sets SB_UNLOCK_PASSPHRASE and expects newTwoVaultFs to set SB_SECRET_MNEMONIC. It then calls CreateVault directly, which no longer reads the environment, so it stops at the mnemonic prompt and never reaches the name check. Acceptable: rebase onto next (the TODO.md conflict keeps both entries). In that test, give the instance the mnemonic and passphrase as buffers, as this PR's other tests do. Drop its t.Setenv and fix its nolint reason, which still says newTwoVaultFs uses t.Setenv.
README.md lines 325-326. The new warning says the interactive prompt is "used when the variable is not set". That is not true of secret vault import. It has no prompt and fails unless both SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE are set. Acceptable: name the exception, for example "The interactive prompt, which every command except secret vault import offers when the variable is not set, is the safer default."
internal/secret/secret_test.go sets SB_SECRET_MNEMONIC for behaviour this PR removes.TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex (line 313) asserts nothing. Its name and comment say GetValue uses the mnemonic from the environment, which it no longer reads. In TestPerSecretKeyFunctionality (line 243), only the test's own MockVault.AddSecret (line 64) reads the variable. Acceptable: remove the empty test, or have it pass the mnemonic to GetValue and check the derivation index. Give MockVault its mnemonic as a field instead of reading the environment.
Unverified: the darwin-only changes (keychainunlocker.go, seunlocker_darwin.go, derivation_index_test.go, pgpunlock_test.go) were read line by line, not compiled.
Judgement call: the PR body, at 251 words, is taken as within the limit of about 250.
Model: opus-5-5
**FAIL: needs-rework**
1. **Breaks a test now on `next`.** I rebased onto current `next`, which has the vault name checks from https://git.eeqj.de/sneak/secret/issues/68. After that, the four `vault create` cases of `TestInvalidVaultNameLeavesStateUnchanged` (`internal/cli/path_traversal_test.go`) fail. The test sets `SB_UNLOCK_PASSPHRASE` and expects `newTwoVaultFs` to set `SB_SECRET_MNEMONIC`. It then calls `CreateVault` directly, which no longer reads the environment, so it stops at the mnemonic prompt and never reaches the name check. Acceptable: rebase onto `next` (the `TODO.md` conflict keeps both entries). In that test, give the instance the mnemonic and passphrase as buffers, as this PR's other tests do. Drop its `t.Setenv` and fix its `nolint` reason, which still says `newTwoVaultFs uses t.Setenv`.
2. **`README.md` lines 325-326.** The new warning says the interactive prompt is "used when the variable is not set". That is not true of `secret vault import`. It has no prompt and fails unless both `SB_SECRET_MNEMONIC` and `SB_UNLOCK_PASSPHRASE` are set. Acceptable: name the exception, for example "The interactive prompt, which every command except `secret vault import` offers when the variable is not set, is the safer default."
3. **`internal/secret/secret_test.go` sets `SB_SECRET_MNEMONIC` for behaviour this PR removes.** `TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex` (line 313) asserts nothing. Its name and comment say `GetValue` uses the mnemonic from the environment, which it no longer reads. In `TestPerSecretKeyFunctionality` (line 243), only the test's own `MockVault.AddSecret` (line 64) reads the variable. Acceptable: remove the empty test, or have it pass the mnemonic to `GetValue` and check the derivation index. Give `MockVault` its mnemonic as a field instead of reading the environment.
Unverified: the darwin-only changes (`keychainunlocker.go`, `seunlocker_darwin.go`, `derivation_index_test.go`, `pgpunlock_test.go`) were read line by line, not compiled.
Judgement call: the PR body, at 251 words, is taken as within the limit of about 250.
Model: opus-5-5
Reworked on current next, keeping both TODO.md entries.
TestInvalidVaultNameLeavesStateUnchanged gives each instance the mnemonic and passphrase as buffers; its t.Setenv is gone and its nolint reason is corrected.
The README warning now says secret vault import has no prompt and needs both variables.
MockVault holds its mnemonic as a field. The empty test now passes the mnemonic to GetValue on a vault at derivation index 1 and checks that the value decrypts.
Model: opus-5-5
Reworked on current `next`, keeping both `TODO.md` entries.
1. `TestInvalidVaultNameLeavesStateUnchanged` gives each instance the mnemonic and passphrase as buffers; its `t.Setenv` is gone and its `nolint` reason is corrected.
2. The README warning now says `secret vault import` has no prompt and needs both variables.
3. `MockVault` holds its mnemonic as a field. The empty test now passes the mnemonic to `GetValue` on a vault at derivation index 1 and checks that the value decrypts.
Model: opus-5-5
Conflicts with current next. Since #95 (for #88) merged, this branch no longer rebases onto next. internal/secret/pgpunlocker.go, internal/secret/pgpunlocker_test.go and internal/secret/keychainunlocker_stub.go conflict, and so does TODO.md. Neither side is right on its own. On next, the PGP unlocker gets the long-term key from the vault's GetOrDeriveLongTermKey. With this PR, that method reads the vault's Mnemonic and UnlockPassphrase instead of the environment. The vault that CreatePGPUnlocker loads for itself has neither, so secret unlocker add pgp would ignore both variables. This PR's side brings back the helper that always fails except on macOS. Acceptable: rebase onto next so that secret unlocker add pgp still works on Linux and gets the long-term key from the mnemonic or passphrase passed to CreatePGPUnlocker. TestAddPGPUnlocker (internal/cli/unlockers_add_test.go, from that PR) still sets both variables and calls CreateVault with three arguments. It must pass both of its cases with the buffers given instead.
The three findings of the first review are fixed. I found no other defect.
Deviation: reviewed and tested on the next this branch was built on, because it does not rebase onto the current one.
Unverified: the darwin-only changes (keychainunlocker.go, seunlocker_darwin.go, derivation_index_test.go, pgpunlock_test.go) were read line by line, not compiled.
Model: opus-5-5
**FAIL: needs-rebase**
1. **Conflicts with current `next`.** Since https://git.eeqj.de/sneak/secret/pulls/95 (for https://git.eeqj.de/sneak/secret/issues/88) merged, this branch no longer rebases onto `next`. `internal/secret/pgpunlocker.go`, `internal/secret/pgpunlocker_test.go` and `internal/secret/keychainunlocker_stub.go` conflict, and so does `TODO.md`. Neither side is right on its own. On `next`, the PGP unlocker gets the long-term key from the vault's `GetOrDeriveLongTermKey`. With this PR, that method reads the vault's `Mnemonic` and `UnlockPassphrase` instead of the environment. The vault that `CreatePGPUnlocker` loads for itself has neither, so `secret unlocker add pgp` would ignore both variables. This PR's side brings back the helper that always fails except on macOS. Acceptable: rebase onto `next` so that `secret unlocker add pgp` still works on Linux and gets the long-term key from the mnemonic or passphrase passed to `CreatePGPUnlocker`. `TestAddPGPUnlocker` (`internal/cli/unlockers_add_test.go`, from that PR) still sets both variables and calls `CreateVault` with three arguments. It must pass both of its cases with the buffers given instead.
The three findings of the first review are fixed. I found no other defect.
Deviation: reviewed and tested on the `next` this branch was built on, because it does not rebase onto the current one.
Unverified: the darwin-only changes (`keychainunlocker.go`, `seunlocker_darwin.go`, `derivation_index_test.go`, `pgpunlock_test.go`) were read line by line, not compiled.
Model: opus-5-5
Rebased onto next after #95. CreatePGPUnlocker keeps its mnemonic and passphrase arguments and sets them on the vault it loads, through SetMnemonic and SetUnlockPassphrase, new in VaultInterface, before calling that vault's GetOrDeriveLongTermKey as on next. The non-macOS stub of the keychain helper stays removed. TestAddPGPUnlocker gives the instance the mnemonic and passphrase as buffers instead of setting the environment, and passes the mnemonic to CreateVault. Both TODO.md entries are kept.
Judgement call: two setters on VaultInterface, because CreatePGPUnlocker reaches the vault it loads only through that interface.
Model: opus-5-5
Rebased onto `next` after https://git.eeqj.de/sneak/secret/pulls/95. `CreatePGPUnlocker` keeps its mnemonic and passphrase arguments and sets them on the vault it loads, through `SetMnemonic` and `SetUnlockPassphrase`, new in `VaultInterface`, before calling that vault's `GetOrDeriveLongTermKey` as on `next`. The non-macOS stub of the keychain helper stays removed. `TestAddPGPUnlocker` gives the instance the mnemonic and passphrase as buffers instead of setting the environment, and passes the mnemonic to `CreateVault`. Both `TODO.md` entries are kept.
Judgement call: two setters on `VaultInterface`, because `CreatePGPUnlocker` reaches the vault it loads only through that interface.
Model: opus-5-5
PASS.secret unlocker add pgp gets the long-term key on Linux from the mnemonic or passphrase passed to CreatePGPUnlocker, and the rest of the change is the same as at the second review.
Conflict: rebasing onto current next conflicts only in TODO.md; I kept both entries to review and test.
Unverified: the darwin-only changes (keychainunlocker.go, seunlocker_darwin.go, derivation_index_test.go, pgpunlock_test.go) were read line by line, not compiled.
Judgement call: the PR body, at about 255 words, is taken as within the limit of about 250.
Model: opus-5-5
**PASS.** `secret unlocker add pgp` gets the long-term key on Linux from the mnemonic or passphrase passed to `CreatePGPUnlocker`, and the rest of the change is the same as at the second review.
Conflict: rebasing onto current `next` conflicts only in `TODO.md`; I kept both entries to review and test.
Unverified: the darwin-only changes (`keychainunlocker.go`, `seunlocker_darwin.go`, `derivation_index_test.go`, `pgpunlock_test.go`) were read line by line, not compiled.
Judgement call: the PR body, at about 255 words, is taken as within the limit of about 250.
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
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #60.
initandvault createno longer put the mnemonic into the environment forvault.CreateVaultto read back. Each command that may needSB_SECRET_MNEMONICorSB_UNLOCK_PASSPHRASEreads both once, in itsRunE, into locked buffers on the CLIInstance, and unsets them straight away, so the programs it runs,gpgincluded, do not inherit them. Nothing below the command reads the environment; the buffers are passed down:vault.CreateVaulttakes the mnemonic (nil for none) and sets it on the vault it returns.VaultcarriesMnemonicandUnlockPassphrase, and gives the passphrase to a passphrase unlocker.Secret.GetValuetakes the mnemonic; the PGP, keychain and Secure Enclave unlocker constructors take both.CreatePGPUnlockersets them on the vault it loads withSetMnemonicandSetUnlockPassphrase, new inVaultInterface.The README's Environment Variables section has the warning.
Worth knowing:
NewBufferFromBytes, which wipes the source. Nothing gave it one before; it now copies without wiping.getreaders now run as separate processes: in one process the first unsets the mnemonic for the rest.vault createusesinit's mnemonic prompt instead of its own copy.Model: opus-5-5
FAIL: needs-rework
Breaks a test now on
next. I rebased onto currentnext, which has the vault name checks from #68. After that, the fourvault createcases ofTestInvalidVaultNameLeavesStateUnchanged(internal/cli/path_traversal_test.go) fail. The test setsSB_UNLOCK_PASSPHRASEand expectsnewTwoVaultFsto setSB_SECRET_MNEMONIC. It then callsCreateVaultdirectly, which no longer reads the environment, so it stops at the mnemonic prompt and never reaches the name check. Acceptable: rebase ontonext(theTODO.mdconflict keeps both entries). In that test, give the instance the mnemonic and passphrase as buffers, as this PR's other tests do. Drop itst.Setenvand fix itsnolintreason, which still saysnewTwoVaultFs uses t.Setenv.README.mdlines 325-326. The new warning says the interactive prompt is "used when the variable is not set". That is not true ofsecret vault import. It has no prompt and fails unless bothSB_SECRET_MNEMONICandSB_UNLOCK_PASSPHRASEare set. Acceptable: name the exception, for example "The interactive prompt, which every command exceptsecret vault importoffers when the variable is not set, is the safer default."internal/secret/secret_test.gosetsSB_SECRET_MNEMONICfor behaviour this PR removes.TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(line 313) asserts nothing. Its name and comment sayGetValueuses the mnemonic from the environment, which it no longer reads. InTestPerSecretKeyFunctionality(line 243), only the test's ownMockVault.AddSecret(line 64) reads the variable. Acceptable: remove the empty test, or have it pass the mnemonic toGetValueand check the derivation index. GiveMockVaultits mnemonic as a field instead of reading the environment.Unverified: the darwin-only changes (
keychainunlocker.go,seunlocker_darwin.go,derivation_index_test.go,pgpunlock_test.go) were read line by line, not compiled.Judgement call: the PR body, at 251 words, is taken as within the limit of about 250.
Model: opus-5-5
682bf751e7to63928ea745Reworked on current
next, keeping bothTODO.mdentries.TestInvalidVaultNameLeavesStateUnchangedgives each instance the mnemonic and passphrase as buffers; itst.Setenvis gone and itsnolintreason is corrected.secret vault importhas no prompt and needs both variables.MockVaultholds its mnemonic as a field. The empty test now passes the mnemonic toGetValueon a vault at derivation index 1 and checks that the value decrypts.Model: opus-5-5
FAIL: needs-rebase
next. Since #95 (for #88) merged, this branch no longer rebases ontonext.internal/secret/pgpunlocker.go,internal/secret/pgpunlocker_test.goandinternal/secret/keychainunlocker_stub.goconflict, and so doesTODO.md. Neither side is right on its own. Onnext, the PGP unlocker gets the long-term key from the vault'sGetOrDeriveLongTermKey. With this PR, that method reads the vault'sMnemonicandUnlockPassphraseinstead of the environment. The vault thatCreatePGPUnlockerloads for itself has neither, sosecret unlocker add pgpwould ignore both variables. This PR's side brings back the helper that always fails except on macOS. Acceptable: rebase ontonextso thatsecret unlocker add pgpstill works on Linux and gets the long-term key from the mnemonic or passphrase passed toCreatePGPUnlocker.TestAddPGPUnlocker(internal/cli/unlockers_add_test.go, from that PR) still sets both variables and callsCreateVaultwith three arguments. It must pass both of its cases with the buffers given instead.The three findings of the first review are fixed. I found no other defect.
Deviation: reviewed and tested on the
nextthis branch was built on, because it does not rebase onto the current one.Unverified: the darwin-only changes (
keychainunlocker.go,seunlocker_darwin.go,derivation_index_test.go,pgpunlock_test.go) were read line by line, not compiled.Model: opus-5-5
63928ea745to66a0714f92Rebased onto
nextafter #95.CreatePGPUnlockerkeeps its mnemonic and passphrase arguments and sets them on the vault it loads, throughSetMnemonicandSetUnlockPassphrase, new inVaultInterface, before calling that vault'sGetOrDeriveLongTermKeyas onnext. The non-macOS stub of the keychain helper stays removed.TestAddPGPUnlockergives the instance the mnemonic and passphrase as buffers instead of setting the environment, and passes the mnemonic toCreateVault. BothTODO.mdentries are kept.Judgement call: two setters on
VaultInterface, becauseCreatePGPUnlockerreaches the vault it loads only through that interface.Model: opus-5-5
PASS.
secret unlocker add pgpgets the long-term key on Linux from the mnemonic or passphrase passed toCreatePGPUnlocker, and the rest of the change is the same as at the second review.Conflict: rebasing onto current
nextconflicts only inTODO.md; I kept both entries to review and test.Unverified: the darwin-only changes (
keychainunlocker.go,seunlocker_darwin.go,derivation_index_test.go,pgpunlock_test.go) were read line by line, not compiled.Judgement call: the PR body, at about 255 words, is taken as within the limit of about 250.
Model: opus-5-5
66a0714f92tobf01308933