secret add, secret import and secret generate secret crashed with memguard's panic when a value did not fit in the locked-memory limit (ulimit -l). The value was held twice, so under the usual 8 MiB limit anything over about 4 MiB crashed. Fixes #145.
secret raises the limit to the hard limit when it starts.
Before its first locked allocation, each of the three commands refuses a limit that leaves fewer than 8 pages for its own keys and buffers.
The value is read a page at a time into a memguard.Stream, which keeps it encrypted, then copied once into a locked buffer. Over 100 MiB gives "secret too large: exceeds 100MB limit"; a value that does not fit in the limit less 8 pages gives "secret too large for the locked-memory limit of N KiB (ulimit -l)". Both exit with status 1, and generate secret checks --length the same way.
Not visible in the diff: the value leaves the stream through Next, because Read re-encrypts a leftover piece, and memguard hangs if it runs out of locked memory there. secret add was measured to need 7 pages besides the value.
Disclosures:
Judgement call: under a limit of 32 KiB or less nothing is stored, not even a short value.
Judgement call: root, which may lock past the limit, is held to it too.
Reading of the definition of done: input of unknown length passes through one locked page before the 100 MiB check.
Deviation: no test checks the refusal over 100 MiB, per sneak's ruling in #52 (comment).
Model: opus-5-5
`secret add`, `secret import` and `secret generate secret` crashed with memguard's panic when a value did not fit in the locked-memory limit (`ulimit -l`). The value was held twice, so under the usual 8 MiB limit anything over about 4 MiB crashed. Fixes https://git.eeqj.de/sneak/secret/issues/145.
- `secret` raises the limit to the hard limit when it starts.
- Before its first locked allocation, each of the three commands refuses a limit that leaves fewer than 8 pages for its own keys and buffers.
- The value is read a page at a time into a `memguard.Stream`, which keeps it encrypted, then copied once into a locked buffer. Over 100 MiB gives "secret too large: exceeds 100MB limit"; a value that does not fit in the limit less 8 pages gives "secret too large for the locked-memory limit of N KiB (ulimit -l)". Both exit with status 1, and `generate secret` checks `--length` the same way.
Not visible in the diff: the value leaves the stream through `Next`, because `Read` re-encrypts a leftover piece, and memguard hangs if it runs out of locked memory there. `secret add` was measured to need 7 pages besides the value.
Disclosures:
- Judgement call: under a limit of 32 KiB or less nothing is stored, not even a short value.
- Judgement call: root, which may lock past the limit, is held to it too.
- Reading of the definition of done: input of unknown length passes through one locked page before the 100 MiB check.
- Deviation: no test checks the refusal over 100 MiB, per sneak's ruling in https://git.eeqj.de/sneak/secret/issues/52#issuecomment-126698.
Model: opus-5-5
internal/cli/secrets.go line 35 (lockedMemoryReservePages = 16), used by checkLockedMemoryLimit at line 281: the 16-page reserve refuses every value when the locked-memory limit is 64 KiB or less (the default of older Linux kernels). On next, under ulimit -l 64, printf hunter2 | secret add x and secret generate secret x succeed. On this branch both fail with "secret too large for the locked-memory limit of 64 KiB". The commands need about 6 pages besides the value. The issue asked only to refuse values that do not fit. Acceptable: a reserve close to what the commands really use, so a short secret is still stored under a 64 KiB limit, with README.md line 135 and the TODO.md entry changed to match.
internal/cli/secrets.go lines 307 to 349 (readSecretFromReader): the limit is checked only after the whole value has gone through the stream, and the stream needs locked memory itself. If the limit is too small for that, secret add and secret import still crash. At 20 KiB they panic. At 16 KiB they hang and ignore Ctrl-C and SIGTERM, because memguard deadlocks when it cannot lock memory for its key. On next, the same command panics at 16 KiB and stores the value at 20 KiB. Acceptable: check the limit against what the command needs before its first locked allocation, so a limit that is too small always gives the error and exit status 1.
README.md line 132: "A secret's value is held in locked memory, which is never written to swap." The value also passes through ordinary memory. secret get decrypts it into an ordinary slice (internal/secret/crypto.go line 96), age copies it into its own buffer while secret add encrypts it, and secret generate secret builds it as a Go string. Acceptable: say that the value is kept in locked memory while secret works on it and so counts against the limit, without promising that it never reaches swap.
internal/cli/secrets_size_test.go line 391: the "over 100 MB gets secret too large" part of the definition of done is tested only for secret generate secret. The issue's own reproduction, secret add of 100 MiB + 1 bytes under a limit below 100 MiB, panics on next and has no test here. Acceptable: the same refusal check for secret add (and secret import) with a value of 100 MiB + 1 bytes.
Model: opus-5-5
1. `internal/cli/secrets.go` line 35 (`lockedMemoryReservePages = 16`), used by `checkLockedMemoryLimit` at line 281: the 16-page reserve refuses every value when the locked-memory limit is 64 KiB or less (the default of older Linux kernels). On `next`, under `ulimit -l 64`, `printf hunter2 | secret add x` and `secret generate secret x` succeed. On this branch both fail with "secret too large for the locked-memory limit of 64 KiB". The commands need about 6 pages besides the value. The issue asked only to refuse values that do not fit. Acceptable: a reserve close to what the commands really use, so a short secret is still stored under a 64 KiB limit, with `README.md` line 135 and the `TODO.md` entry changed to match.
2. `internal/cli/secrets.go` lines 307 to 349 (`readSecretFromReader`): the limit is checked only after the whole value has gone through the stream, and the stream needs locked memory itself. If the limit is too small for that, `secret add` and `secret import` still crash. At 20 KiB they panic. At 16 KiB they hang and ignore Ctrl-C and SIGTERM, because memguard deadlocks when it cannot lock memory for its key. On `next`, the same command panics at 16 KiB and stores the value at 20 KiB. Acceptable: check the limit against what the command needs before its first locked allocation, so a limit that is too small always gives the error and exit status 1.
3. `README.md` line 132: "A secret's value is held in locked memory, which is never written to swap." The value also passes through ordinary memory. `secret get` decrypts it into an ordinary slice (`internal/secret/crypto.go` line 96), age copies it into its own buffer while `secret add` encrypts it, and `secret generate secret` builds it as a Go string. Acceptable: say that the value is kept in locked memory while `secret` works on it and so counts against the limit, without promising that it never reaches swap.
4. `internal/cli/secrets_size_test.go` line 391: the "over 100 MB gets secret too large" part of the definition of done is tested only for `secret generate secret`. The issue's own reproduction, `secret add` of 100 MiB + 1 bytes under a limit below 100 MiB, panics on `next` and has no test here. Acceptable: the same refusal check for `secret add` (and `secret import`) with a value of 100 MiB + 1 bytes.
Model: opus-5-5
The reserve is 8 pages and the value is counted in whole pages (secret add was measured at 7 besides the value with both SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE set); a new test stores a short secret under 64 KiB. README updated; this PR no longer changes TODO.md (#173).
The three commands check the limit before reading those two variables, their first locked allocation; tested at 20 KiB.
README: the value is kept in locked memory while secret works on it and counts against the limit; swap is no longer mentioned.
Added secret add and secret import of 100 MiB + 1 bytes under a 1 MiB limit, read from a sparse file.
Model: opus-5-5
Rework:
1. The reserve is 8 pages and the value is counted in whole pages (`secret add` was measured at 7 besides the value with both `SB_SECRET_MNEMONIC` and `SB_UNLOCK_PASSPHRASE` set); a new test stores a short secret under 64 KiB. README updated; this PR no longer changes `TODO.md` (https://git.eeqj.de/sneak/secret/issues/173).
2. The three commands check the limit before reading those two variables, their first locked allocation; tested at 20 KiB.
3. README: the value is kept in locked memory while `secret` works on it and counts against the limit; swap is no longer mentioned.
4. Added `secret add` and `secret import` of 100 MiB + 1 bytes under a 1 MiB limit, read from a sparse file.
Model: opus-5-5
internal/cli/secrets_size_test.go lines 408 to 412 and 425 to 432: three cases check that a value over 100 MiB is refused (secret add and secret import of a 100 MiB + 1 byte sparse file, and secret generate secret with --length of 100 MiB + 1). sneak ruled against these tests in #52 (comment). That ruling says no test may check that an oversized secret is refused, nothing replaces the cases deleted then, and no test moves more than about 1 MiB of secret data. The add and import cases push 100 MiB through the binary. This replaces finding 4 of the earlier review on this PR. Acceptable: remove the three cases, the sparse file and the words "and one over 100 MB" from the test's comment (lines 394 to 395). The locked-memory-limit cases stay.
internal/cli/secrets_size_test.go lines 217 to 233: the name and comments of TestAddSecretBufferGrowth ("buffer growth strategy", "initial 4KB", "first double", "second double", "more doublings") describe the doubling read buffers that this PR removes. Acceptable: rename the test and reword its comments for what it now checks, which is values just under, at and just over one read of one memory page. Dropping the test is also acceptable.
Model: opus-5-5
1. `internal/cli/secrets_size_test.go` lines 408 to 412 and 425 to 432: three cases check that a value over 100 MiB is refused (`secret add` and `secret import` of a 100 MiB + 1 byte sparse file, and `secret generate secret` with `--length` of 100 MiB + 1). sneak ruled against these tests in https://git.eeqj.de/sneak/secret/issues/52#issuecomment-126698. That ruling says no test may check that an oversized secret is refused, nothing replaces the cases deleted then, and no test moves more than about 1 MiB of secret data. The add and import cases push 100 MiB through the binary. This replaces finding 4 of the earlier review on this PR. Acceptable: remove the three cases, the sparse file and the words "and one over 100 MB" from the test's comment (lines 394 to 395). The locked-memory-limit cases stay.
2. `internal/cli/secrets_size_test.go` lines 217 to 233: the name and comments of `TestAddSecretBufferGrowth` ("buffer growth strategy", "initial 4KB", "first double", "second double", "more doublings") describe the doubling read buffers that this PR removes. Acceptable: rename the test and reword its comments for what it now checks, which is values just under, at and just over one read of one memory page. Dropping the test is also acceptable.
Model: opus-5-5
Removed the three cases over 100 MiB, the sparse file, and "and one over 100 MB" from the test's comment. The PR body now says in one line that this refusal has no test.
Renamed TestAddSecretBufferGrowth to TestAddSecretAroundPageSize. Its comments now describe values around one and two 4 KiB pages, the amount secret add reads at a time, and no longer mention buffer doubling.
Model: opus-5-5
Rework:
1. Removed the three cases over 100 MiB, the sparse file, and "and one over 100 MB" from the test's comment. The PR body now says in one line that this refusal has no test.
2. Renamed `TestAddSecretBufferGrowth` to `TestAddSecretAroundPageSize`. Its comments now describe values around one and two 4 KiB pages, the amount `secret add` reads at a time, and no longer mention buffer doubling.
Model: opus-5-5
secret add, import and generate secret crashed with memguard's panic once a
value outgrew the locked-memory limit, and because the value was held twice,
half the usual 8 MiB limit was enough. secret now raises the limit to the hard
limit at startup. Each of the three commands refuses, before its first locked
allocation, a limit that leaves fewer than 8 pages for its keys and buffers.
The value is read a page at a time into a memguard.Stream, which keeps it
encrypted, and copied once into locked memory after its size is checked
against 100 MiB and the limit. It leaves the stream through Next, not Read:
Read re-encrypts a leftover piece, and memguard hangs if that runs out of
locked memory.
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.
secret add,secret importandsecret generate secretcrashed with memguard's panic when a value did not fit in the locked-memory limit (ulimit -l). The value was held twice, so under the usual 8 MiB limit anything over about 4 MiB crashed. Fixes #145.secretraises the limit to the hard limit when it starts.memguard.Stream, which keeps it encrypted, then copied once into a locked buffer. Over 100 MiB gives "secret too large: exceeds 100MB limit"; a value that does not fit in the limit less 8 pages gives "secret too large for the locked-memory limit of N KiB (ulimit -l)". Both exit with status 1, andgenerate secretchecks--lengththe same way.Not visible in the diff: the value leaves the stream through
Next, becauseReadre-encrypts a leftover piece, and memguard hangs if it runs out of locked memory there.secret addwas measured to need 7 pages besides the value.Disclosures:
Model: opus-5-5
41b0ccc41btof081505f78f081505f78toef10bddddaef10bddddato28606075c6internal/cli/secrets.goline 35 (lockedMemoryReservePages = 16), used bycheckLockedMemoryLimitat line 281: the 16-page reserve refuses every value when the locked-memory limit is 64 KiB or less (the default of older Linux kernels). Onnext, underulimit -l 64,printf hunter2 | secret add xandsecret generate secret xsucceed. On this branch both fail with "secret too large for the locked-memory limit of 64 KiB". The commands need about 6 pages besides the value. The issue asked only to refuse values that do not fit. Acceptable: a reserve close to what the commands really use, so a short secret is still stored under a 64 KiB limit, withREADME.mdline 135 and theTODO.mdentry changed to match.internal/cli/secrets.golines 307 to 349 (readSecretFromReader): the limit is checked only after the whole value has gone through the stream, and the stream needs locked memory itself. If the limit is too small for that,secret addandsecret importstill crash. At 20 KiB they panic. At 16 KiB they hang and ignore Ctrl-C and SIGTERM, because memguard deadlocks when it cannot lock memory for its key. Onnext, the same command panics at 16 KiB and stores the value at 20 KiB. Acceptable: check the limit against what the command needs before its first locked allocation, so a limit that is too small always gives the error and exit status 1.README.mdline 132: "A secret's value is held in locked memory, which is never written to swap." The value also passes through ordinary memory.secret getdecrypts it into an ordinary slice (internal/secret/crypto.goline 96), age copies it into its own buffer whilesecret addencrypts it, andsecret generate secretbuilds it as a Go string. Acceptable: say that the value is kept in locked memory whilesecretworks on it and so counts against the limit, without promising that it never reaches swap.internal/cli/secrets_size_test.goline 391: the "over 100 MB gets secret too large" part of the definition of done is tested only forsecret generate secret. The issue's own reproduction,secret addof 100 MiB + 1 bytes under a limit below 100 MiB, panics onnextand has no test here. Acceptable: the same refusal check forsecret add(andsecret import) with a value of 100 MiB + 1 bytes.Model: opus-5-5
28606075c6toa3a0d44a9dRework:
secret addwas measured at 7 besides the value with bothSB_SECRET_MNEMONICandSB_UNLOCK_PASSPHRASEset); a new test stores a short secret under 64 KiB. README updated; this PR no longer changesTODO.md(#173).secretworks on it and counts against the limit; swap is no longer mentioned.secret addandsecret importof 100 MiB + 1 bytes under a 1 MiB limit, read from a sparse file.Model: opus-5-5
a3a0d44a9dto9fd0505a93internal/cli/secrets_size_test.golines 408 to 412 and 425 to 432: three cases check that a value over 100 MiB is refused (secret addandsecret importof a 100 MiB + 1 byte sparse file, andsecret generate secretwith--lengthof 100 MiB + 1). sneak ruled against these tests in #52 (comment). That ruling says no test may check that an oversized secret is refused, nothing replaces the cases deleted then, and no test moves more than about 1 MiB of secret data. The add and import cases push 100 MiB through the binary. This replaces finding 4 of the earlier review on this PR. Acceptable: remove the three cases, the sparse file and the words "and one over 100 MB" from the test's comment (lines 394 to 395). The locked-memory-limit cases stay.internal/cli/secrets_size_test.golines 217 to 233: the name and comments ofTestAddSecretBufferGrowth("buffer growth strategy", "initial 4KB", "first double", "second double", "more doublings") describe the doubling read buffers that this PR removes. Acceptable: rename the test and reword its comments for what it now checks, which is values just under, at and just over one read of one memory page. Dropping the test is also acceptable.Model: opus-5-5
9fd0505a93tob7b951a42bRework:
TestAddSecretBufferGrowthtoTestAddSecretAroundPageSize. Its comments now describe values around one and two 4 KiB pages, the amountsecret addreads at a time, and no longer mention buffer doubling.Model: opus-5-5
b7b951a42btoac2ada66b4Review passed.
Model: opus-5-5