WALM-297: Owner-scoped bearer token auth for the read API (Phase 1) - #546
Open
harrymove-ctrl wants to merge 2 commits into
Open
harrymove-ctrl wants to merge 2 commits into
harrymove-ctrl wants to merge 2 commits into
Conversation
…e 1)
Console can never call WALM-295's owner-scoped read endpoints under the
existing Ed25519 signed-request scheme (auth.rs::verify_signature) because
it structurally never holds a delegate key — WALM-298's identity-link flow
proves control of an owner address entirely on Console's own side and never
grants signing capability. This adds the missing bridge: after Console has
proven control of owner Y itself, it authenticates as a trusted client via
a single shared service credential (team decision: service credential over
mTLS/signed-client-assertion, for lower implementation cost on both sides)
and mints a short-lived, owner-scoped bearer token limited to memories.read.
- owner_token_auth.rs: HMAC-signed opaque token mint/verify, mirroring the
existing security_delete_auth.rs token scheme; a FromRequestParts
extractor for token-gated routes.
- routes/owner_token.rs: POST /v1/owner-tokens (issuance, gated by a
constant-time service-credential check, Sui-address validation, and a
MemWalAccount existence check) plus GET /v1/owners/{owner}/_token_probe,
a minimal token-gated example route standing in for WALM-295's real
handlers, which don't exist on this branch yet (fresh branch off dev).
Wiring the OwnerToken extractor into the real namespaces/memories/agents
handlers is the small follow-up once this branch merges with WALM-295.
- rate_limit.rs: independent per-credential and per-owner issuance budgets,
fail-closed on Redis error.
- types.rs / routes/remember.rs: new Config fields + config-fixture updates.
- docs/api/owner-token-auth.md: full contract for Console, including the
four distinct 429/503 response shapes across the two independent rate
limiters and two independent unavailability paths, and an explicit note
on the shared-secret trust boundary (a leaked credential can mint a
memories.read token for any owner — accepted Phase-1 trade-off).
Verified end-to-end against a live local build (real HTTP, not just unit
tests): credential rejection, cross-owner 403, real expiry, forged tokens
with altered permissions/audience correctly rejected (proving the scope
check isn't hardcoded), rate-limit trip, and no secret leakage across
logs/responses. 321/321 lib tests, 432/432 bin tests, no regressions.
…doc drift An adversarial multi-dimension review of commit 9c16e46 confirmed 2 major and 5 minor findings; the 2 major ones are fixed here, along with 3 of the cheap minors. 1. (major) POST /v1/owner-tokens had no throttling on guessing the shared service credential: the credential-gate middleware rejected bad guesses in-process before the per-credential rate limiter ever ran, and that limiter is keyed by the guessed value anyway, so a varying guess got a fresh Redis bucket every time. Added owner_token_ip_rate_limit_middleware (mirrors accounts_rate_limit_middleware/sponsor_rate_limit_middleware's unconditional per-IP layer) as the true outermost layer on this route, independent of credential validity. 2. (major) token_probe's actual authorization logic — owner-match via same_owner and the memories.read permission-scope check, explicitly documented as the copy-paste template WALM-295's real read handlers will use — had zero test coverage despite needing no AppState/DB/ Redis to test. Added 5 unit tests covering the match/mismatch and present/missing-scope/empty-scope cases. 3. (minor) docs/api/owner-token-auth.md's "validation order" bullet had the service-credential check listed after Sui-address-format, when in the real request pipeline the credential gate is the outermost middleware and runs first. Corrected, and documented the new IP rate-limit layer. 4. (minor) expires_at used chrono's plain to_rfc3339(), which always renders the UTC offset as "+00:00" — never matching the "Z"-suffixed example the response's own doc comment and the API doc promised. Switched to to_rfc3339_opts(SecondsFormat::Secs, true). Also replaced the silent "fall back to now()" on timestamp-overflow with a real error, since silently reporting a valid token as already expired is worse than failing loudly. 5. (minor) OWNER_TOKEN_TTL_SECS had no upper bound (only rejected 0), which both defeats the "short-lived" security property the token scheme's threat model rests on and could, for extreme values, push the expires_at computation outside chrono's representable range. Clamped to a new MAX_OWNER_TOKEN_TTL_SECS ceiling (24h). Not fixed here (left as follow-ups, all minor/test-coverage-only, noted in review): OWNER_TOKEN_AUDIENCE not deployment-scoped (relies on OWNER_TOKEN_SECRET differing per environment, an operational convention); no test for duplicate-nonce-still-verifies (intentional per the nonce design note, but undertested); no test pinning the per-owner-vs-per-credential rate-limiter independence; no test pinning find_account_by_owner's DB-error path surfacing as 500 rather than being swallowed. Re-verified after all fixes: cargo check --lib / --bin clean, 321/321 lib tests + 437/437 bin tests pass (up from 432 — 5 new tests), no regressions.
Collaborator
Style Guide AuditAudited 1 file(s) against the Sui Documentation Style Guide. 25 violation(s) found. All must be fixed before merge.
|
3 tasks
Collaborator
Author
Re-review — WALM-297All 5 acceptance criteria verified against the diff. Approve, one item to close before merge:
To close before merge: the "small follow-up once this branch merges with WALM-295/#537" (wiring Depth: targeted (auth/rate-limit surface, single new subsystem). |
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
3 participants
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.
Summary
Console can never call WALM-295's owner-scoped read API (
GET /v1/owners/{owner}/{namespaces,memories,agents}) under the existing Ed25519 signed-request scheme, because it structurally never holds a delegate key — WALM-298's identity-link flow proves control of an owner address entirely on Console's own side and never grants signing capability.This adds the missing bridge: after Console has proven control of owner
Yitself, it authenticates as a trusted client via a single shared service credential (team decision — service credential over mTLS/signed-client-assertion, for lower implementation cost on both sides) and mints a short-lived, owner-scoped bearer token limited tomemories.read.Full contract:
docs/api/owner-token-auth.md.What's here
owner_token_auth.rs— HMAC-signed opaque bearer token mint/verify, mirroring the existingsecurity_delete_auth.rstoken scheme; aFromRequestPartsextractor for token-gated routes.routes/owner_token.rs—POST /v1/owner-tokens(issuance) +GET /v1/owners/{owner}/_token_probe, a minimal token-gated example route standing in for WALM-295's real handlers, which don't exist on this branch yet (fresh branch offdev). Wiring theOwnerTokenextractor into the realnamespaces/memories/agentshandlers is the small follow-up once this branch merges with WALM-295.rate_limit.rs— three independent budgets on issuance: per-IP (throttles credential guessing, runs before the credential check), per-service-credential, per-owner. All fail closed on Redis error.types.rs/routes/remember.rs— newConfigfields + fixture updates.docs/api/owner-token-auth.md— full contract for Console: request/response shapes, all distinct 429/503 response shapes across the three limiters, the shared-secret trust-boundary note.Testing
Verified end-to-end against a live local build (real HTTP, not just unit tests): credential rejection, cross-owner 403, real token expiry, forged tokens with altered permissions/audience correctly rejected (proving the scope check isn't hardcoded), rate-limit trip, no secret leakage across logs/responses.
Then reviewed via an adversarial multi-dimension pass (security, correctness, API-contract-vs-code, test-coverage). Confirmed findings fixed in a follow-up commit:
token_probe's actual authorization logic (owner-match + permission-scope check — the literal security decision this feature exists to make, and the template WALM-295 will copy) had zero test coverage → added 5 unit tests.expires_attimestamp format matching its own documentedZ-suffixed contract, TTL upper bound).cargo check --lib/--bin: clean.cargo test: 321/321 lib + 437/437 bin, no regressions.Not yet done (follow-ups, non-blocking)
OwnerTokeninto WALM-295's real read handlers (needs that branch present).OWNER_TOKEN_AUDIENCEisn't deployment-scoped — cross-environment isolation currently relies onOWNER_TOKEN_SECRETdiffering per environment (an operational convention, not code-enforced).