Conversation
Closes item 6 of #325 and the first of its two friction points. ## Item 6 was mostly already done `Denylist` is documented at length in `docs/jwt.md` (setup, store example, `ErrTokenRevoked`, fail-closed, sizing entries to `exp`) and again in section 6 of `docs/secure-login.md`. The issue says it "appears nowhere in `docs/secure-login.md`", and that is no longer true. The one place it really was missing is the README module table, where the jwt row listed rotation and stopped. That row now names the denylist. That is the whole of item 6, and I would rather say so than manufacture work around a claim that has aged out. The other friction point needed nothing either: `docs/key-management.md` already covers environment-provided key material under "Sourcing keys without a volume", with `os.Getenv` in the example. ## The rotation caveat was genuinely undocumented Nothing in `docs/` mentioned the parallel-request race, and it is real: a frontend that fans out several fetches on one navigation can have two of them refresh with the same token, and the second arrives after the stored hash has been replaced. It gets a 401 for a token that was valid when it was sent. `CreateTokens`, `HashRefreshToken` and `VerifyRefreshTokenHash` already support staying non-rotating; now the docs say so, with the code. They also say what it costs, because "rotation is optional" without that is worse than silence. **Reuse detection**, and where it actually lives. `RotateTokens` detects nothing: it verifies the presented token and reissues. The detection is the caller atomically replacing the stored hash, so a replayed old token fails the lookup. Drop rotation and you drop that signal. **The denylist stops killing the whole session at once.** This one I did not expect, and it is written down nowhere. `RotateTokens` carries the original `jti` forward; `CreateTokens` mints a fresh one per call. Measured on a fixed clock: | Call | SessionID | |---|---| | `CreateTokens` | `018fd3ab-c200-771c-b0e0-17fa236d71af` | | `CreateTokens` again | `018fd3ab-c200-7115-a937-fd114f66224d` | | `RotateTokens` on the first pair | `018fd3ab-c200-771c-b0e0-17fa236d71af` | `denylist.go` promises that one entry kills the whole session "because the jti is stable across rotations". True, and it holds only while you rotate. Refresh by calling `CreateTokens` and each refresh starts a new `jti`, so the caller must store the newest `SessionID` and revoke that one, while access tokens minted under an earlier `jti` stay valid until their own `exp` (one `AccessTokenTTL`, 15 minutes by default). That is a different guarantee, not a broken one. But a reader told "just add a denylist" would never find it, so `denylist.go` now carries the caveat next to the promise, and `docs/jwt.md` explains the trade in both directions. ## Verification The session-id table above is measured, not read off the source: I drove `CreateTokens` twice and `RotateTokens` once against a fixed clock and printed the ids. My first draft of this section said to "compensate with a Denylist" and would have shipped advice that quietly does not work, which is why it is here as a table rather than a claim. `go build`, `go vet`, `gofmt -l` clean, `go test -race ./...` 10 of 10 packages. The only code change is a doc comment. ## Still open in #325 `auth/field` and `auth/credential`. The latter is in progress on a separate branch. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…d activation (#338) Closes item 3 of #325. Mints a high-entropy token, hands back the raw value once for the email link and a hash for the caller to store, and verifies a presented token in constant time with the TTL enforced. Pure computation, so it keeps the no-database contract. This is what powers password reset and account activation, the two most security sensitive emails an application sends. `docs/secure-login.md` walked the login path and stopped before both. | Closed | Open | |---|---| | 32 bytes of CSPRNG entropy, base64url so it drops into a link | `TTL`, default 1 hour, capped at 24 | | HMAC-SHA256 with the library's pepper, length-prefixed input | | | Constant-time comparison, run before the expiry check | | | The purpose and subject binding | | The TTL cap is not arbitrary: a reset link that outlives a day is a standing key to the account sitting in an inbox. ## The binding is the point A token minted to reset a password must not be redeemable to activate an account, and one minted for user A must not verify for user B. Both are mistakes a caller makes with one shared table or one lookup by token, and neither should be reachable. Purpose and subject go into the HMAC; `Verify` recomputes with the pair it was given. ## Two defects I found and fixed **The separator was defeatable.** The first draft hashed `purpose || 0x00 || subject || 0x00 || token`, with a comment claiming the zero byte made the encoding unambiguous. It does, but only while no field can contain that byte, and a Go string can. Measured: ``` purpose="a\x00b" subject="c" -> 65350024d7b06dec... purpose="a" subject="b\x00c" -> 65350024d7b06dec... ``` Identical. A caller whose subject came from untrusted input could redeem a token minted for a different pair, which is the exact property this module exists to provide. Each field is now length-prefixed with a big-endian `uint32`, which depends on nothing about the contents. **The module carried per-issue state.** `Issue` also wrote the token and hash onto the receiver. The race detector flags it on two concurrent calls, and it kept the raw token, the one value shown exactly once, alive for the lifetime of the module. That came from an ambiguous instruction of mine, and a test had pinned it as if it were a feature. The fields are gone; `Issued` is the only place the token appears. Both are regression tests now, and both were checked by sabotage: | Sabotage | Reddens | |---|---| | restore the `0x00` separator | the collision test | | restore the receiver fields | the concurrency test, under `-race` | ## Corrected alongside Four places still described the old separator or the receiver side effect: the `Config` doc comment, the `Credential` struct comment, the `Issue` doc comment, and `docs/credential.md`. A comment that outlives its code is a false statement, so all four now say what the code does. ## Three footguns stated in `docs/credential.md` 1. **Single use is the caller's job.** Delete or flag the stored hash when the token is redeemed, in the same transaction that applies the effect. 2. **Changing a password must invalidate outstanding reset tokens for that user.** Otherwise an old link in an old inbox still works after the account has already been recovered, which is the case reset exists to close. 3. **Rate limit issuance per subject**, or the endpoint is a mail bomb aimed at any address an attacker names. `ErrInvalidCredential` and `ErrExpired` are both safe to show, and the docs say to show the same generic message for both: distinguishing them tells an attacker that a token existed. ## Verification `go build`, `go vet`, `gofmt -l` clean, `go test -race ./...` 11 of 11 packages, coverage 97.8 for this package against a floor of 90. Fuzz target clean for 20 seconds, 3.9M executions. ## Still open in #325 `auth/field` only, the AES-256-GCM plus blind index one. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Closes item 2 of #325. **That was the last one open, so #325 closes with this.** AES-256-GCM for a single database column, plus an HMAC-SHA256 blind index so the value stays searchable by exact equality without being readable. The case that drives it: store a user's email encrypted and still enforce one account per address. | Closed | Open | |---|---| | AES-256-GCM, 12 byte random nonce per call | `Context`, and it is required | | HMAC-SHA256 for the blind index | | | HKDF derivation of both keys | | | Length-prefixed AAD and index input | | ## Keys are derived, never reused `Keys().RefreshSecret()` is already the pepper for refresh hashes, api keys, totp recovery codes and credential tokens. Using it directly as an AES key too would put one secret in five jobs, where a weakness in any construction reaches the others. Both keys come from `crypto/hkdf` (standard library on the pinned toolchain, confirmed before writing the design) under versioned labels, so a future change to the derivation can leave the old label available to decrypt existing rows. Measured on one provider: ``` master secret eb9a04e93baae0007654afee... encKey derived 5d334988a119c29c63094ff2... idxKey derived 7015d342c627348db08ce789... ``` ## Context binds a value to its column `Config.Context` names the field being protected and goes into both the GCM additional authenticated data and the blind index, length-prefixed. A ciphertext lifted from the `email` column will not decrypt as `phone`, and an index for one field never matches another. An empty `Context` is refused: silently accepting `""` would put every field in one keyspace. Length prefixing rather than a separator byte, for the reason `auth/credential` documents: a separator disambiguates only while no field can contain it, and a Go string can contain any byte. ## The test that no behaviour can see Nothing in a round trip notices whether the keys were derived. Remove `deriveKey`, use the master secret for both the AEAD and the index, and **every existing test still passes**: encryption still round trips, indexes still match. That is the shape of #229, where seventeen controls could be deleted with the suite staying green. So `TestDeriveKey_SeparatesTheTwoKeysFromEachOtherAndFromTheMaster` asserts the keys directly: both 32 bytes, neither equal to the master, not equal to each other, and deterministic across calls, because a non-deterministic derivation would lose every stored row on restart. Sabotaged by making `deriveKey` return the master secret, and it is the only test that goes red. ## Sabotages | Sabotage | Reddens | |---|---| | AAD dropped | both cross-context tests | | nonce derived from the plaintext | the ciphertext-uniqueness test | | length prefix removed from the index | both collision tests | | `deriveKey` returns the master secret | the separation test, and only that one | Three earlier attempts were **inert** because my patterns did not match the code, and a first go at the nonce one broke the build on an unused import. Diffing the file before reading the result caught all four. An inert sabotage reads exactly like a test that works, and a sabotage that does not compile proves nothing. ## Wire format `base64(nonce || sealed)`. Measured 28 bytes of overhead on a 17 byte plaintext, which is 12 of nonce plus 16 of GCM tag. ## Four things `docs/field.md` states outright 1. **The blind index leaks equality, on purpose.** Anyone with the database sees which rows share a value. Do not index a low-entropy field where confirming a guess is the whole attack. 2. **Normalisation is the caller's.** The module does not lowercase or trim: `auth/email` and `auth/username` already do, and a module that normalised silently would disagree with whatever the caller stored. 3. **Losing the key loses the data.** No recovery path. 4. **Rotating the key means re-encrypting every row**, and the library does not do it for you. The migration shape is written out. ## Verification `go build`, `go vet`, `gofmt -l` clean, `go test -race ./...` 12 of 12 packages, coverage 90.5 for this package against a floor of 90. Both fuzz targets clean. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Jaro-c
added
type:feature
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
1 participant
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.
Release
v1.13.0. MINOR: two new modules, nothing existing broken, no call site changes.Together with #335 in v1.12.0 this closes #325 in full, the six gaps found dogfooding the library against a production Go application.
What a consumer gets
auth/credential(#338). Single-use tokens for password reset and account activation, the two most security sensitive emails an application sends. Mints a token, hands back the raw value once for the link and a hash to store, verifies in constant time with the TTL enforced. Purpose and subject are bound into the hash, so a reset token cannot be redeemed as an activation and one minted for user A cannot verify for user B.auth/field(#339). AES-256-GCM for a single column, plus an HMAC-SHA256 blind index so a value stays searchable by exact equality without being readable. Both keys are derived from the managed secret with HKDF under versioned labels rather than reusing it.Contextbinds a ciphertext to the column it was written for.docs/jwt.mdgained a "When not to rotate" section (#337). Refresh rotation is still the default, but a frontend that fans out fetches on one navigation can race two refreshes and sign the user out for nothing. The non-rotating path was always supported and is now documented, along with what it costs.The measurement that section exists for
RotateTokenscarries the originaljtiforward;CreateTokensmints a fresh one per call:CreateTokens018fd3ab-c200-771c-b0e0-17fa236d71afCreateTokensagain018fd3ab-c200-7115-a937-fd114f66224dRotateTokenson the first pair018fd3ab-c200-771c-b0e0-17fa236d71afSo
denylist.go's promise that one entry kills a whole session holds only while you rotate. A deployment that refreshes withCreateTokensmust store the newestSessionID, and access tokens under an earlierjtisurvive to their ownexp. That caveat now sits next to the promise in the source, not only in the docs.Two defects found and fixed before either module shipped
auth/credential's separator was defeatable. The first draft hashedpurpose || 0x00 || subject || 0x00 || token. A separator disambiguates only while no field can contain it, and a Go string can:Every field is length-prefixed now, in both modules.
auth/credentialcarried per-issue state.Issuewrote the token and hash onto the module receiver, which the race detector flags on concurrent calls and which kept the raw token alive for the module's lifetime. A test had pinned it as a feature.Nothing to do on upgrade
New(p)is unchanged in every module. Both new modules are opt in, and neither touches existing key material.Verification
Eight sabotages across the two modules, each reverted, each reddening only what it should. Four attempts were inert or failed to build and were caught by diffing the file before reading the result.
The one worth naming: remove
deriveKeyfromauth/fieldand use the master secret for both the AEAD and the index, and every behavioural test still passes. Key separation is invisible from outside, so it is asserted directly, and that assertion is the only thing that goes red.go build,go vet,gofmt -lclean,go test -race ./...12 of 12 packages. Every package above the coverage floor of 90: credential 97.8, field 90.5. Four fuzz targets clean.