Conversation
Moves the `require` in all nine example modules from `v1.16.0` to `v1.16.1`, which the module proxy has served since 20:11 UTC today. Only the authcore lines change in `go.mod` and `go.sum`; nothing else was tidied in. Each example builds and vets on its own with `GOWORK=off` against the published version, and `go build`, `go vet` and `go test` pass for all nine through `examples/go.work`. v1.16.1 changes no API, so no code changes. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
… tags off main (#461) The GitHub Release now carries what the release pull request says. Before this, `release.yml` published `--generate-notes` alone, which is a list of titles, so v1.15.0 and v1.16.0 went out with no breaking callout and v1.16.1 with no mention of its behaviour change until I added them by hand (#460). **`scripts/release-notes.sh <commit>`** finds the merged pull request into `main` whose merge commit is `<commit>` and prints its body without the DCO trailer. It fails, printing nothing, when there is no such pull request, when the body is empty, or when the pull request is labelled `breaking` and has no `## Breaking` section. #397 and #450, the two breaking releases so far, both have one. **`release.yml`**: - a new first step refuses a tag whose commit is not on `main` (`git merge-base --is-ancestor`), which the release contract asks for and nothing here checked; - the notes are composed before build and test, and `gh release create` gets them through `--notes`, with `--generate-notes` appending the list underneath; - the job gets `pull-requests: read` for that, and its checkout gets `persist-credentials: false`, since it publishes through `gh` and never pushes with git; - the comment about curated notes in a `CHANGELOG.md` this repository does not have is gone. **Tests.** `scripts/release-notes.test.sh` runs the script against a stubbed `gh` whose fixture is a real `gh pr list` response for #458, #450 and #397 captured today, with variants that each change one thing. 25 assertions, each refusal paired with an acceptance of the same shape. It runs as a new `release-notes` job in `ci.yml` on every push and pull request. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | drop the `## Breaking` check | 3 assertions fail | | drop the empty-body check | 4 fail | | stop removing the trailer | 6 fail | | stop removing carriage returns | 1 fails | | accept any pull request instead of the one with this merge commit | 11 fail | | drop `--base main` from the query | 1 fails | | strip `Signed-off-by` lines anywhere, not just at the end | 1 fails | **Run against the real repository**, read-only: the two new steps of `release.yml`, extracted as they are, produced 17 lines of notes for `v1.16.1` and 45 for `v1.16.0`, and a local tag on `e3ae07f` (on `develop`, not on `main`) was refused with `zz-probe-offmain points at e3ae07f…, which is not on main`. **Not verified until the next tag**, and they fail differently: - that `pull-requests: read` is enough for the job token. If not, "Compose release notes" fails before anything is published; - that `--notes` plus `--generate-notes` puts the notes above the list, which is how `gh release create --help` documents it. If not, the release is published with the notes in the wrong place, and I fix the page with `gh release edit`, which works on these immutable releases (done three times today). shellcheck 0.10.0 and actionlint are clean on every changed file. Closes #460 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Tests for two controls that could be deleted with the suite green, and for one error path nothing exercised (#462). No production behaviour changes; the only non-test edit is a comment. **`internal/keymanager`** (`readcap_stream_test.go`): - `readCapped` on `/dev/zero`, which Stat reports as zero bytes, must be refused by the read-time cap and not by the Stat check. Paired with regular files at the cap (read whole) and one byte past it (the Stat refusal). - End to end, a provisioned set whose `refresh_secret.key` is replaced by a link to `/dev/zero`: `Load` and `New` both refuse it and the private key is untouched. The refusal comes from the regular-file check in `fsguard.go`, before any read, and the test asserts that one. - `New` and `Load` on a `KeysDir` below a regular file report `inspect key directory "<dir>"` and create nothing. - Where `/dev/zero` does not exist the two stream tests skip and say what was not exercised. **`auth/apikey`**: `validateConfig(Config{})` is refused with "prefix must not be empty", paired with a one-character prefix that passes. A comment on the check says why it stays although `New` never reaches it: without it, an empty prefix passes every other rule. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | disable the read-time cap in `readCapped` | `TestReadCapped_refusesAStreamLongerThanTheCap` fails | | disable the regular-file check in `fsguard.go` | the end-to-end test fails, and its message shows the read-time cap caught the device instead | | skip the inspect error in `New` | `TestKeysDirBelowARegularFileIsReported` fails | | disable the empty-prefix check | `TestValidateConfig_refusesAnEmptyPrefix` fails | The second row is the defence in depth working: with the first barrier gone, the second still refused. `internal/keymanager` goes from 90.5% to 91.0%, `auth/apikey` from 93.6% to 94.9%. `gofmt`, `go vet ./...`, `go test -race -count=1 ./...` and gosec are clean. Closes #462 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…again (#465) Closes the two gaps in #464. - **`azp`**: after the exact-audience check, a present `azp` must equal the client id. This restores what #426 removed. - **The signing key's `issuer`**: the JWK decoder keeps the `issuer` member, the cache hands the selected key back with it (`candidateFor`, with `key` as a thin wrapper for existing callers), and `checkKeyIssuer` requires the token's `iss` to equal it. A `{tenantid}` template is completed from the token's `tid`, and a template key with no `tid` is refused. A key without the member (every Google and Discord key in the captured JWKS) is unrestricted, as before. I checked the captured provider documents before relying on this: every key in Microsoft's common JWKS carries the v2 form (`https://login.microsoftonline.com/<tenant or {tenantid}>/v2.0`), which is the only issuer form the Microsoft preset and `AzureMultiTenantIssuer` accept, so no token that verified legitimately is refused now. `VerifyIDToken`'s doc comment, the "What it guarantees" list in `docs/oauth.md` and the `ErrIDTokenInvalid` row in `docs/errors.md` say so. Tests in `idtoken_binding_test.go`, each refusal paired with the same token shape that verifies: - `azp` naming another client is refused; `azp` equal to the client id verifies; - a key pinned to the consumer tenant refuses a token for another tenant and verifies one for its own; - a `{tenantid}` key verifies when `iss` and `tid` name the same tenant, refuses when they differ, and refuses a token without `tid`; - a key without `issuer` verifies under the fixed issuer check. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | skip the `azp` comparison | `refusesAnotherClientsAZP` fails | | make `checkKeyIssuer` return early | the tenant and template tests fail | | stop completing the template | the template test fails | | stop requiring `tid` for a template key | the template test fails on the missing-`tid` case | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...` and gosec are clean; `auth/oauth` is at 92.9%. Closes #464 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…d harden the JWKS outage paths (#467) Fixes the six defects in #466, one change each: 1. `UserInfo` refuses a 200 whose object carries `error`, and an empty object, with `ErrUserInfo`. 2. Basic credentials are form-encoded (`url.QueryEscape`) before `SetBasicAuth`, per RFC 6749 section 2.3.1. The usual secret shape (letters, digits, `-._~`) is unchanged by the encoding, and a test pins that. 3. `applyDefaults` copies `Provider.AuthMethods`, as it already did `Scopes`. 4. `VerifyIDToken` refuses any byte outside the compact JWS alphabet before parsing and parses with `WithStrictDecoding`, so a token has one spelling. 5. A stale known kid gets the same one-minute cooldown after a failed refresh as an unknown one, returning `ErrJWKSStale` (which still matches `ErrJWKS`) without fetching. The acceptance half of `TestJWKS_failsClosedAfterExpiry` now steps past the cooldown before expecting a successful refresh. 6. A caller whose joined fetch ended in `context.Canceled` while its own context is live runs the fetch again. The retry covers the case where the caller that started the fetch has since gone away; it does not change what happens on a timeout. The `Exchange` and `Provider.AuthMethods` comments now say that Post is sent when nothing is advertised, which is what the code does. Tests, each refusal paired with an acceptance of the same shape: - `lenient_paths_test.go`: error object and `{}` refused, a real profile returned; Basic credentials read back through `url.QueryUnescape` by the test server for a hostile and an ordinary secret; the caller's slice changed after `New` without effect; a newline and flipped unused bits in the signature refused, the token as signed accepted. - `jwks_outage_internal_test.go`: five lookups inside the cooldown make no fetch, and one past it fetches exactly once and serves the key; and, under `testing/synctest` so the ordering is deterministic, a caller parked on a fetch whose starter is cancelled gets the key after one retry. Both ran three times under `-race`. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | accept the error object / the empty object | the UserInfo test fails (both) | | send Basic credentials raw | `formEncodesBasicCredentials` fails | | drop the `AuthMethods` copy | `copiesTheAdvertisedAuthMethods` fails | | drop the alphabet check / drop strict decoding | `refusesASecondSpellingOfTheToken` fails (both) | | drop the stale-kid cooldown | `staleKidWaitsOutTheCooldown…` fails | | drop the retry | `joinedCallerRetries…` fails | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean; `auth/oauth` is at 93.1%. Closes #466 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
… encoding (#469) `New` now refuses a `PreviousPublicKeys` entry that anyone could forge signatures under (#468). `checkVerificationKey` (`auth/jwt/pubkey.go`), standard library only: - the encoded `y` must be below `2^255 - 19` (canonical); - `y = 1` is the identity and is refused; - any other point is mapped to its Montgomery `u = (1+y)/(1-y)` and run through `crypto/ecdh` X25519, which clamps the scalar to a multiple of 8 and returns an error on the all-zero output. That is exactly the case of a point of order 1, 2, 4 or 8. I chose this over a hard-coded blocklist so the check does not rest on constants copied from memory, and over `filippo.io/edwards25519` to avoid a new dependency. The refusal is `ErrInvalidConfig` with `previous public key <i> cannot be trusted: <reason>`, and the `PreviousPublicKeys` doc comment says so. **Tests** (`pubkey_test.go`). The oracle is the forgery itself, independent of how the check decides: a key is dangerous when `ed25519.Verify` accepts `R = identity, S = 0` for one of 64 messages. - Refused: all zero, all zero with the sign bit, the identity, `y = p-1`, the three non-canonical encodings `p`, `p+1`, `2^255-1`, and both order-8 points. The test asserts the oracle really forges under each canonical small-order key, the two order-8 encodings included, so a wrong constant would fail the test instead of passing it. - Accepted: 200 generated keys, with the oracle unable to forge under them. - End to end: `New` with the all-zero key is `ErrInvalidConfig`; with a real previous key it succeeds. **Sabotage**, each edit confirmed to change the file: | Sabotage | Result | |---|---| | skip the check in `validateConfig` | `TestNew_refusesAForgeablePreviousKey` fails | | skip the canonical check | the three non-canonical cases fail | | skip the identity check | the identity case fails, and the code panics on it (`ModInverse(0)` is nil) | | skip the small-order check | the small-order cases and the `New` test fail | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...` and gosec are clean; `auth/jwt` is at 94.1%. Closes #468 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
… to regenerate a recorded set (#471) Fixes #470 in three commits. **Key material behind one more pointer.** `KeyManager` now holds `material *secretMaterial` instead of the three slices. fmt reaches a `*KeyManager` inside an unexported field without calling its `Format`, and on a verb pointers do not support it dereferences exactly one level. That level now ends at an address. The redaction methods are unchanged. I did not add `Format` methods to `AuthCore` and the stores, because they would not cover a caller's own struct that holds a `Config`. `format_leak_test.go` formats the `*AuthCore` and `AuthCore` on an in-memory store, the `Config` carrying the `KeyStore`, `ac.Config()`, the `KeyStore` itself and an `*AuthCore` on the disk store, with 15 verbs. It searches each output for the private key and the refresh secret in decimal, both hex cases and the `%#v` form. Run against the code before this change, it reports 84 leaks; with it, none. **A recorded key set is not regenerated.** When the key files are all gone and `metadata.json` is present (it is written only after a set is published), `New` refuses. The message names the recorded key id, says to restore from a backup, and says that deleting `metadata.json` is the deliberate way to start over. The message does not tell anyone to delete key files. The test checks the refusal, that no key file was created and the marker is byte-identical, and that deleting the marker then yields a new set. **`metadata.json` must be a regular file.** `readMetadata` stats the path and refuses anything else before opening it, so a FIFO no longer blocks `New` or `Load`. The test (unix only, since it needs `mkfifo`) runs both with a 10 s guard. **The committed binary is removed**, and `/authcore-keygen` and `/cmd/authcore-keygen/authcore-keygen` are ignored. Rebuilding it at the root now leaves `git status` clean. The three tagged releases that contain it cannot change. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | let an empty set with a marker regenerate | `TestNewRefusesToRegenerateWhenMetadataRecordsAKeySet` fails | | drop the regular-file check on the marker | the FIFO test fails for `New` and `Load` after its 10 s guard | | the formatting fix | checked against the previous code instead: 84 leaks found. A sabotage that flattens the struct does not compile, so it proves nothing | Each commit builds on its own. `gofmt`, `go vet ./...`, `go test -race -count=1 ./...` and gosec are clean. `internal/keymanager` is at 90.8% and the root package at 96.1%. Closes #470 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…cept one spelling per token (#473) Fixes #472 in three commits, each passing its own tests. **jwt: revocation, leeway, zero values, typed-nil denylist** - `RotateTokens` consults the `Denylist` after verifying the refresh token and returns `ErrTokenRevoked` for a revoked session. `RotateTokensContext` is new (additive) and passes the caller's context to the lookup; `RotateTokens` uses the same 5-second default as `VerifyAccessToken`. A store error fails closed. - `docs/jwt.md` now sizes the entry to outlive the session (until the newest refresh token's expiry plus leeway) and says to delete the stored refresh hash in the same step. Rotation is refused while the entry exists, so no later refresh token can appear. - `ClockSkewLeeway` is capped at 5 minutes; a larger value is `ErrInvalidConfig`. **A configuration with a larger leeway now fails at `New`.** Five minutes is the usual clock-skew tolerance, and anything past it extends `exp` beyond what the TTL ceilings allow. - `CreateTokens`, `VerifyAccessToken[Context]` and `RotateTokens[Context]` return `ErrNotInitialised` on a zero value or a nil pointer instead of panicking. - `New` refuses a `Denylist` holding a nil pointer. - The `RefreshTokenExpiresAt` wording no longer claims it is when the user must log in again; under rotation each refresh issues a new one. **jwt and field: one spelling per token or ciphertext** - Both verifiers share one parser option list, `parserOptions`, which adds `WithStrictDecoding`, so the access and refresh paths cannot drift apart again. `checkTokenString` refuses any byte outside the compact JWS alphabet before parsing, with `ErrTokenMalformed`. - `field.Decrypt` refuses a ciphertext that does not re-encode to itself. **apikey**: `ParseID` returns `ErrNotInitialised` on a zero value or a nil pointer. Tests pair each refusal with the case that still works: rotation of a revoked session, of an active one, and with a failing store; the leeway exactly at and 1 ns past the ceiling; a typed-nil denylist; every method on a zero value and a nil pointer; a line break and a flipped unused bit in access tokens, refresh tokens and ciphertexts, next to the originals; a zero-value `ParseID`. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | skip the denylist in `RotateTokensContext` | `refusesARevokedSession` fails | | drop the leeway ceiling | `boundsTheClockSkewLeeway` fails | | drop the typed-nil check | `refusesATypedNilDenylist` fails | | drop the `CreateTokens` guard | the zero-value test fails (panics on the nil pointer) | | drop the alphabet check / drop strict decoding | `IssuedTokensHaveOneSpelling` fails (each) | | drop the canonical check in `Decrypt` | `refusesASecondSpellingOfTheCiphertext` fails | | drop the `ParseID` guard | `ParseID_zeroValueRefuses` fails (panics) | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean. `auth/jwt` 94.5%, `auth/field` 92.9%, `auth/apikey` 95.0%. Closes #472 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…ix the otpauth label (#475) Fixes #474 in three commits, one per package group, each passing the tests of all four packages. **password, username**: `New` refuses a nil provider and a nil logger, and `password.New` refuses more than one `Config`, all as `ErrInvalidConfig`. These are the guards #425 added to the other four modules. **totp** - `New` refuses a nil provider, logger or `Keys()`, a second `Config`, and a refresh secret that is not 32 bytes. - `New` copies `SkewSteps` before validating it, so the caller's int no longer controls a built module. - The otpauth label is written as text and escaped once; a colon inside the issuer or the account is escaped too, so the one literal colon stays the separator. `TestEnroll_URI_RoundTrips` and `TestEnroll_URI_AwkwardAccountName` pinned the double escaping and now assert the raw label and its split. - `decodeSecret` accepts only the spelling `Enroll` hands out. - The log line prints the skew value, and the `New` and `applyDefaults` comments describe the pointer semantics. - `docs/totp.md`: recovery codes are consumed by a conditional `DELETE` whose affected-row count decides success. The page now says why the read-modify-write of the whole list is wrong, and the footguns list repeats it. **email** - `New`/`NewWithConfig` refuse a nil provider or logger, with a new `email.ErrInvalidConfig` (added to `docs/errors.md`). - An all-digit top-level label is refused. - `VerifyDomain` refuses a domain past DNS size limits with `ErrInvalidEmail` before touching the cache or the resolver. - The shared MX lookup runs under `context.WithoutCancel(ctx)` bounded by a 10-second module timeout, so the caller that started it can give up without deciding the answer for the others. Each caller's own context still bounds its wait, and the doc comment says so. Tests, each refusal paired with what is accepted: - constructor guards in all four packages; - `SkewSteps` written to 1,000,000 after `New` while an hour-old code stays refused and the current one verifies; - line breaks in a TOTP secret; - `127.0.0.1` and `1.2` refused, `123.example.com` and `example.co1` accepted; - oversized domains refused with the resolver never called and nothing cached; - a starter cancelled mid-lookup while a joined caller still gets `ErrDomainNoMX`. This one runs under `testing/synctest`, so the ordering is deterministic. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | drop the nil-provider guard (password, username, email) | each constructor test fails, panicking | | drop the extra-config refusal (password) | the constructor test fails | | drop the `SkewSteps` copy | `copiesSkewSteps` fails | | drop the secret-length check (totp) | the constructor test fails | | drop the canonical-secret check | `refusesLineBreaks` fails | | build the URI through `url.URL` again | both URI tests fail | | drop the all-digit check / the size check | their tests fail | | run the lookup on the caller's context | the cancellation test fails | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean. Coverage: password 95.7%, totp 95.1%, email 94.4%, username 94.8%. Closes #474 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…d pin the unpinned controls (#477) Closes #476 in three commits, each building and passing on its own. **keymanager** - `decodePEMBlock` is the one parser behind both key files and `FromPEM`: the input must start with a PEM block, the block must be labelled `PRIVATE KEY` or `PUBLIC KEY` as expected, carry no headers, and be the only thing in the input apart from whitespace. The refresh-secret error no longer wraps the hex package's error, which quotes a byte of the file. **A key file with text before or after its block, or with headers, is refused at startup now.** Files written by authcore, by `openssl genpkey`/`pkey -pubout`, and by `authcore-keygen` are unaffected. - The permission warning covers write bits (mask `0o066`), so a secret file others can replace is reported, and `Load` warns when `KeysDir` is group- or world-writable without the sticky bit (a Kubernetes Secret mount is `1777` and is not reported). `New` still tightens the directory instead. - A parent directory that cannot be opened for its fsync (search but no read permission) is a warning on the first run, as it already was on the load path; the test that pinned the refusal is rewritten to the new contract. - The two comments claiming `exists` makes callers "fail closed rather than generating" now say what it feeds. **KeyID**: `New` refuses a `KeyStore` whose `Keys` report an empty `KeyID`, and `jwt.New` refuses a `PreviousPublicKeys` entry whose id equals the current key's, with `ErrInvalidConfig`, instead of letting it replace the current key in the verification set. The `Keys.KeyID` contract and `docs/key-management.md` say so. I did not require a custom store to reproduce the built-in derivation: it is an internal detail, and a store with its own stable ids has worked and keeps working. **docs**: `docs/faq.md` no longer tells an operator to delete `.authcore`; `docs/containers.md` describes concurrent first start as measured today rather than as "being reworked". **Tests** - `mode_table_test.go`: {`New`, `Load`} × {private key, refresh secret} × {0640, 0604, 0602, 0620, 0600, 0400}, exactly one warning naming the file for the four open modes and none otherwise; `Load` on a 0777 and 0733 directory warns, on 1777 (sticky), 0700 and 0750 does not, and the mode is left as found. - `review_round2_internal_test.go`: `New` on a 0555 directory leaves it at 0500 and 0750 at 0700; recovery with an unrelated staging directory that sorts first still completes from the one whose private key matches; seven non-canonical PEM inputs refused with the reason named and the canonical pair (with surrounding whitespace) accepted; the not-hex error names no byte of the file and a hex file of the same length loads. - `TestNewWarnsWhenTheParentCannotBeSynced`, `TestNew_refusesAnEmptyKeyID` (with a custom non-empty id accepted), `TestNew_refusesAPreviousKeyWithTheCurrentKeyID` (with a different key registered). Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | drop the secret warning on `New` / the private-key warning on `Load` | the mode table fails on those cells | | mask group only / read only | the 0604 and 0602/0620 cells fail | | `Chmod(dir, dirMode)` instead of `mode&dirMode` | the tighten test fails | | accept any staging directory (`\|\| true`) | the recovery test fails | | drop the type, header, trailing-content or prefix check | the matching PEM cases fail | | wrap the hex error again | the no-echo test fails | | refuse the parent sync again / drop the directory warning | their tests fail | | accept an empty `KeyID` / a previous key with the current id | their tests fail | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean. `internal/keymanager` is at 90.3%, the root package at 96.2%, `auth/jwt` at 94.5%. The keymanager figure sits close to the floor because the strict parser and the directory warning add branches; the untested ones are the Windows returns and the non-permission parent-sync failure, which has no fault seam. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…cal email local parts (#479) Closes #478 in four commits, one per package, each building and passing on its own. **password** - `isPrintable` also refuses U+2800 BRAILLE PATTERN BLANK and the code points in Unicode's `Other_Default_Ignorable_Code_Point` (the Hangul fillers, U+034F and the like), which `unicode.IsPrint` admits although they render as nothing. Variation selectors stay accepted: they modify a visible base. The package doc and `docs/password.md` say what is refused. **A new password holding one of these is refused now**; existing hashes verify as before, since `Verify` applies no policy. - Tests pin what the review found unpinned: `Hash` normalises to NFC (NFD register, NFD and NFC sign-in); all four `*bool` policy snapshots, by flipping the caller's variable after `New`; the `Memory` and `Iterations` ceilings at `New`, with the value at the ceiling accepted; and a stored hash with a bare label (`$v$`, `$m$`, ...) refused with `ErrInvalidHash` rather than panicking. **totp** - Every method refuses on a zero value or a nil pointer: `Enroll`, `VerifyStep` and `Verify` with `ErrNotInitialised`, `VerifyRecoveryCode` with `(0, false)`, and **`HashRecoveryCode` now returns `(string, error)`** so it can report it, the change #425 made to `apikey.Hash` for the same reason. The sentinel is in `docs/errors.md`. - `FuzzVerify` has an oracle: RFC 4226 written apart from the package (HMAC-SHA1, dynamic truncation, six digits) decides which codes the fixed clock's window accepts, and `VerifyStep` and `Verify` (with an in-memory recorder) must agree exactly. Seeds cover every step of the window and one outside it on each side. **email** - The local part has one canonical spelling: NFC, then lowercase. Control (Cc, C1 included), format, line and paragraph separator and default-ignorable characters in it are refused, and so is a non-ASCII letter whose lowercase is ASCII (U+0130 to `i`). U+212A KELVIN SIGN is canonically equivalent to K and becomes `k` through NFC, which the test states. **`josé@` now stores as `josé@`**, so a row stored from decomposed input before this change no longer matches; I know of none. - `evictExpired`, which nothing in production called, is gone; the eviction inside `store` is tested with a full cache of expired entries, and an empty MX answer with no error is pinned as `ErrDomainNoMX`. The `maxCacheSize` comment no longer describes a background goroutine. **username**: the fuzz target asserts the `[a-z0-9_-]` set, the length bounds and the reserved list on every accepted value, with seeds that put a disallowed byte between allowed ones, since the start and end rules refuse the old seeds before `isAllowed` sees them. Sabotage, each edit confirmed to change the file: | Sabotage | Result | |---|---| | drop NFC in `Hash` | `TestHash_normalisesTheInputItHashes` fails | | drop any one of the three policy snapshots | its subtest fails | | drop the memory or the iterations ceiling | `refusesWorkFactorsPastTheCeilings` fails | | drop the label prefix check | the bare-label test fails, with the old panic | | drop the default-ignorable or the braille refusal | the blank-rendering test fails | | drop NFC / the control refusal / the ASCII-fold refusal in the local part | the matching local-part test fails | | drop the eviction / the empty-answer branch | their tests fail | | drop the `Enroll`, `HashRecoveryCode` or `VerifyRecoveryCode` guard | the zero-value test fails | | make `stepMatches` accept every code | `FuzzVerify` fails on its seeds | | make `isAllowed` accept every byte | `FuzzValidateAndNormalize` fails on the new seeds | `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean. Coverage: password 98.8%, totp 95.4%, email 95.9%, username 94.8%. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…tion and blind-index guides (#481) Closes #480 in three commits: two are tests only, one is documentation and a package doc comment. No production behaviour changes. **Guides** - `docs/secure-login.md` section 5 replaces the refresh hash with the conditional `UPDATE ... WHERE id = $2 AND refresh_hash = $3` and a row-count check, the recipe `docs/jwt.md` step 5 got in #427, and says that reuse detection depends on it. - The `auth/field` package doc's read path recomputes the index of the decrypted value and compares it with the one looked up; the sentence claiming an index hit proves the plaintext is gone. - `docs/configuration.md` describes the credential binding as the length-prefixed construction the code uses; `docs/errors.md` gains the `jwt.ErrTokenOversized` and `jwt.ErrNotInitialised` rows; `docs/key-management.md` says which modules share one HMAC key and why that waits for a release that can change stored hashes. **jwt tests** - The claim checks: a token without `exp` and one issued a day in the future are refused on the access path (the signer fixture now carries a UUIDv7 `jti`, since #443's shape check refused the old `"jti"` first and left these tests green with the option deleted) and on the refresh path; each is paired with the same token accepted. `mapJWTError` collapses the reason into `ErrTokenInvalid`, so the acceptance twin is what proves which check fired. - The leeway through `RotateTokens` (20 s past `exp` accepted with a 30 s leeway, 40 s refused); the refresh-token issuance cap with a 5806-byte issuer refused and a 5600-byte one issuing and rotating; `validateConfig` refusing an empty audience directly, which `New` never reaches; the zero-value `VerifyRefreshTokenHash` guard against the hash an empty key produces, which the old fixture (`"any-hash"`) never reached. - `FuzzIsUUIDv7` compares against RFC 9562 written as a regular expression apart from the code, with seeds for a moved dash, hex digits where the dashes go, a non-hex byte and a short input; `FuzzVerifyAccessToken` accepts only the issued access token and requires every refusal to be a documented sentinel. **apikey, credential, field tests** - Known answers for every stored format, with the expected HMAC and HKDF values computed outside Go under a fixed secret: the apikey hash, the refresh-token hash, the credential hash (length-prefixed purpose, subject, token) and the blind index; and a ciphertext produced today that must keep decrypting, which pins the encryption label, the AAD and the nonce placement. - `FuzzParseID` seeds that reach the lowercase-hex rule and the prefix check; a zero-value `apikey.Hash` refused. Sabotage, each edit confirmed to change the file and to compile: | Sabotage | Result | |---|---| | drop `WithExpirationRequired` / `WithIssuedAt` from `parserOptions` | the access and refresh tests for that claim fail | | ignore the leeway | both leeway tests fail | | drop the zero-value guard in `VerifyRefreshTokenHash` / in `apikey.Hash` | their tests fail | | drop the dash, variant or hex check in `isUUIDv7` | `FuzzIsUUIDv7` fails on a seed | | widen `isHex` to A-F / drop the prefix check in `ParseID` | `FuzzParseID` fails on a seed | | accept a refresh token in `VerifyAccessToken` | `FuzzVerifyAccessToken` fails on the refresh seed | | drop the empty-audience check | `TestValidateConfig_refusesAnEmptyAudience` fails | | change the blind-index label / drop the AAD / swap the credential fields / change the apikey or refresh hash input | the matching known-answer test fails | The refresh-token issuance cap has a test; its sabotage was not run separately because the shared verifier refuses the oversized token on rotation anyway, so the test fails under it through that path. `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec and the nine example builds are clean. Coverage: jwt 95.9%, apikey 96.2%, credential 98.4%, field 92.9%. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…ent the multi-tenant client (#483) Closes #482 in three commits. Tests and docs only; the one production edit is the doc comment above `oauth.Microsoft`. **Tests** (one file per control, except where an existing table already covered the neighbours): - `entropy_test.go`: state, nonce and verifier each decode to 32 bytes and are 43 characters. - `idtoken_time_claims_test.go`: no `exp` is refused with "exp claim is required", `iat` an hour ahead with "token used before issued", next to a token that verifies. - `presets_fixture_test.go`: three prefix and suffix variants of a tenant issuer are refused by `AzureMultiTenantIssuer`, the bare tenant issuer is accepted. - `redirect_internal_test.go`: `:8443` on the same host is cross-origin, `:443` is the same origin. - `exchange_non2xx_test.go`: a 500 with a token-shaped body fails on the status, the same body at 200 succeeds. - `discover_https_test.go`: a plaintext issuer is refused with a transport that counts zero calls; an `http://` `jwks_uri` in the document is refused and the `https://` one is returned. - `response_caps_test.go`, `response_caps_jwks_internal_test.go`: a body of exactly 1 MiB is read, one byte more fails with `ErrExchange`, `ErrJWKS`, `ErrDiscovery` and `ErrUserInfo`. - `oauth_url_presets_test.go`: the auth URL carries a stale value for all eight generated parameters, and each comes back once with the fresh value. - `oauth_more_test.go`: `UserInfo` on an OIDC client asserts `ErrNoUserInfo`, not any error. - `issuer_predicate_test.go`: a predicate that approves everything still refuses an empty `iss`. - `jwks_fuzz_test.go`: an oracle on every accepted key and a real Google RSA seed (details in the commit). **Docs**: `docs/oauth.md` shows the hand-built multi-tenant Azure `Provider` and says why neither the preset nor `Discover` on the alias can build one; the setup snippet handles every error. `docs/errors.md` gains the `oauth.ErrJWKSStale` row. `TestDocumentedMultiTenantProviderIsAccepted` builds the documented `Config`. Sabotage, each edit confirmed to change the file, `go test ./auth/oauth/`: | Sabotage | Result | |---|---| | `randomTokenBytes = 1` | `TestAuthCodeURL_tokensCarry32RandomBytes` fails | | drop `WithExpirationRequired` / `WithIssuedAt` | the matching `timeClaimsEnforced` subtest fails | | drop `^` / `$` from the Azure pattern | `TestPreset_AzureMultiTenantIssuer` fails (each) | | `sameOrigin` ignores the port | `TestSafeRedirect/same_host,_other_port` fails | | skip the Exchange status check | `refusesNon2xxEvenWithSuccessBody/rejection` fails | | skip the Discover issuer / endpoint https check | its test fails (each) | | raise each of the four read caps | its `OverCap` test fails (each) | | `q.Add` for `code_challenge_method` | `dedupsPreExistingQueryParameters` fails | | UserInfo returns another error without a URL | `TestUserInfo_noURLConfigured` fails | | drop `iss == ""` before the predicate | `issuerPredicateRejectsEmptyIss/empty_iss_is_refused` fails | | RSA floor 2048 to 16 bits | `FuzzParseJWK` fails on seed #3, also in a plain `go test` run | | skip the EC on-curve check | `go test -fuzz=FuzzParseJWK` fails within a second | My first sabotage of the Exchange status check was `false && a || b`, which still refused a 500; rewritten with parentheses before reading the result. `gofmt`, `go vet ./...`, `go test -race -count=1 ./...`, gosec, staticcheck on the touched files and the nine example builds are clean. `auth/oauth` is at 93.5%. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Jaro-c
added
type:security
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.
This release is the second round of the September review: every package got a fresh read, and what it found is fixed here. One signature changes and several inputs that used to pass are refused now, so read the section below before upgrading.
Breaking
totp.(*TOTP).HashRecoveryCodereturns(string, error)(fix: refuse zero-value totp, blank password characters and non-canonical email local parts #479). It reportsErrNotInitialisedon a zero value or a nil pointer instead of panicking, the same changeapikey.Hashgot in v1.15.0. Callers add the error check.jwt.Config.ClockSkewLeewayabove 5 minutes fails atNewwithErrInvalidConfig(fix(jwt): refuse to rotate revoked sessions, bound the leeway, and accept one spelling per token #473). Anything past five minutes extendedexpbeyond the TTL ceilings.RotateTokensconsults theDenylist(fix(jwt): refuse to rotate revoked sessions, bound the leeway, and accept one spelling per token #473). A refresh token whose session is revoked is refused withErrTokenRevoked, and a store error fails closed.RotateTokensContextis new and passes your context to the lookup.PRIVATE KEY/PUBLIC KEY), no headers, nothing before or after it except whitespace. Files written by authcore,authcore-keygenandopenssl genpkey/pkey -puboutare unaffected; a hand-assembled file with comments around the block is refused at startup.Newrefuses aKeyStorethat reports an emptyKeyID, andjwt.Newrefuses aPreviousPublicKeysentry with the current key's id (fix(keymanager): parse key files strictly, warn on writable files, and pin the unpinned controls #477), both withErrInvalidConfig. The second used to replace the current key in the verification set.Verifyapplies no policy.a@127.0.0.1anda@x.1no longer validate.Newrefuses to regenerate a key set thatmetadata.jsonrecords (fix(keymanager): keep key material out of formatted output and refuse to regenerate a recorded set #471). When every key file is gone and the marker is still there, startup fails and names the recorded key id: restore the files from a backup, or deletemetadata.jsonto start over on purpose. It used to generate a fresh set silently, which logged every user out and made everyauth/fieldcolumn unreadable.\rand\n, so it used to decode to the same key. A secret stored exactly asEnrollreturned it is unaffected.totp.Newalso refuses a refresh secret that is not 32 bytes.password,username,totpandemail, andpassword.Newrefuses more than oneConfig(fix: harden the password, totp, email and username constructors and fix the otpauth label #475), all withErrInvalidConfig.Security fixes
jwt: a previous public key of small order or with a non-canonical encoding is refused (fix(jwt): refuse previous public keys of small order or non-canonical encoding #469). Access tokens, refresh tokens andfieldciphertexts accept one spelling each (fix(jwt): refuse to rotate revoked sessions, bound the leeway, and accept one spelling per token #473).oauth: ID tokens are bound to the issuer the signing key is restricted to, andazpis checked again (fix(oauth): bind ID tokens to the signing key's issuer and check azp again #465). A userinfo 200 carrying an error object or no profile is refused, Basic client credentials are form-encoded, and the JWKS outage paths no longer fetch on every call or inherit another caller's cancellation (fix(oauth): refuse non-profile userinfo, encode Basic credentials, and harden the JWKS outage paths #467).keymanager: key material stays out of formatted output, a recorded key set is never regenerated (fix(keymanager): keep key material out of formatted output and refuse to regenerate a recorded set #471), and writable key files and directories are warned about (fix(keymanager): parse key files strictly, warn on writable files, and pin the unpinned controls #477).totp: every method refuses on a zero value instead of panicking,NewcopiesSkewSteps, and the otpauth label is escaped once with a colon in the issuer or account escaped too (fix: harden the password, totp, email and username constructors and fix the otpauth label #475, fix: refuse zero-value totp, blank password characters and non-canonical email local parts #479).email:VerifyDomainrefuses a domain past DNS size limits before the cache or the resolver, and a caller that gives up no longer cancels the MX lookup for the others (fix: harden the password, totp, email and username constructors and fix the otpauth label #475).Other changes
jwt,oauth,keymanagerandapikey(test: reach the read-time key file cap and the empty-prefix check #463, test(jwt): pin the claim checks and every stored format; fix the rotation and blind-index guides #481, test(oauth): pin the controls a mutation pass could delete, and document the multi-tenant client #483).docs/oauth.mdshows a multi-tenant Azure client that builds;docs/errors.mdlistsoauth.ErrJWKSStale(test(oauth): pin the controls a mutation pass could delete, and document the multi-tenant client #483).Signed-off-by: Jaro-c 75870284+Jaro-c@users.noreply.github.com