Conversation
Consolidate Postgres OAuth persistence into pkg/oauth/store, add SQLite parity with JSON payload tables (migration 0014_oauth.sql), and wire builtin OAuth through runtime.WithBuiltinOAuth and haistack serve config. Includes Inferno reference testkit, public /fhir/metadata with OAuth auth, SMART well-known mirroring under /fhir, and PEM signing key persistence. Co-authored-by: Adegoke Adewoye <[email protected]>
- Wire SMARTBearerBundleResolver for scope enforcement on haistack serve - Renumber Postgres OAuth migration to 0017 (fix version-15 collision) - Add NewSQLiteServer, move postgres store test to pkg/oauth/store - Document oauth config in haistack/runtime READMEs; update examples - Add HAISTACK_OAUTH_AUTO_APPROVE, serve startup discovery URL - Restrict registration token to Authorization Bearer header Co-authored-by: Adegoke Adewoye <[email protected]>
…B keys, tenants, session login)
Implement the remaining OAuth port plan items with no intentional deferrals:
- RFC 7662 /oauth/introspect with confidential client auth and refresh token lookup
- DB-backed rate limits (SQLite/Postgres migrations + store implementations)
- Encrypted DB signing keys with OAUTH_SIGNING_KEY_ENCRYPTION_SECRET and PEM fallback
- Multi-tenant routes at /t/{tenantId}/ via MultiTenantServer
- Production session login at /oauth/login with OAUTH_SESSION_SECRET
- Wire all features in runtime.WithBuiltinOAuth and HTTP root handler
Includes tests and updated OAuth/haistack operations documentation.
Co-authored-by: Adegoke Adewoye <[email protected]>
cursor
Bot
changed the title
… defaults - Fix gofmt on smart-oauth, builder, infernotest reference - OAUTH_SIGNING_KEY_ROTATE startup rotation for DB signing keys - Background consent-session purge wired in runtime.Start - ApplyProductionDefaults + RequirePKCEForAllClients for production - Config validation for OAUTH_SIGNING_KEY_ENCRYPTION_SECRET and OAUTH_SESSION_SECRET - Docs updates (runtime README, smart-auth-architecture, OPERATIONS) - Tests for session auth, signing key crypto, production defaults, tenant routes Co-authored-by: Adegoke Adewoye <[email protected]>
…I lint - Add MultiTenantBearerAuth issuer-aware FHIR bearer validation - Port redirect URI validation and DCR scope allow-list - Fix golangci-lint: keys.go Close errcheck, remove unused redis script - Add wire, redirect URI, and DCR registration tests Co-authored-by: Adegoke Adewoye <[email protected]>
Add issuer column migrations and persist issuer on authorization codes, refresh tokens, and pending consent sessions. AuthorizationStore consume and lookup methods take the server issuer so multi-tenant deployments with a shared database cannot cross-exchange codes or refresh tokens. Co-authored-by: Adegoke Adewoye <[email protected]>
Require non-empty issuer on SQL/Redis saves and strict issuer filters on consume. Scope Redis keys by issuer segment. Harden refresh introspection. Expand issuer binding tests for refresh, consent, authorize flow, and Redis. Co-authored-by: Adegoke Adewoye <[email protected]>
Drop FileAuthorizationStore, FileClientStore, FileTokenRevocationStore, ProductionStores, and NewProductionServer. Durable OAuth state is SQLite or Postgres via pkg/oauth/store; memory remains for tests. Keep ProductionPaths only for PEM signing key fallback used by runtime. Co-authored-by: Adegoke Adewoye <[email protected]>
File-backed OAuth stores were removed from production; finish that by deleting the remaining FileAuthorizationStore (it referenced a deleted atomic helper) and the issuer-on-save test that used it. Memory, SQL, and Redis already enforce issuer-scoped keys and Lua consume-before-delete. Co-authored-by: Adegoke Adewoye <[email protected]>
Give auth codes, refresh tokens, and pending sessions composite primary keys (issuer, id) so tenants can share a store without colliding. Redis Lua consume checks issuer and exp (unix millis) before delete, using one shared helper. Memory consume deletes expired rows instead of leaving them. Co-authored-by: Adegoke Adewoye <[email protected]>
Rename ProductionPaths to SigningKeyPaths to match PEM-only disk state. Add a SQLite server restart test that reuses the same issuer, client, and authorization code. Document that pkg/smart file stores are backend-assertion helpers, not authorization-server persistence. Co-authored-by: Adegoke Adewoye <[email protected]>
Drop unused SigningKeyPaths.StateDir, default SigningKID to haistack, and assert PEM load via LoadSigningKey. Restart coverage now includes auth-code exchange then a second restart that consumes the refresh token. Co-authored-by: Adegoke Adewoye <[email protected]>
Restore memory-store and SQL migration files that were staged with the PEM/refresh restart test by accident. Co-authored-by: Adegoke Adewoye <[email protected]>
Lua already gates consume on exp. Re-checking ExpiresAt in Go after a successful delete can reject a token that was just burned if the two clocks ever disagree. LookupRefreshToken and GetPendingAuthorization stay non-destructive. exp remains Redis-only JSON via marshalBoundJSON, not a Go struct field. Co-authored-by: Adegoke Adewoye <[email protected]>
Review:
|
| Item | Notes |
|---|---|
| Default DCR scope | Omitting scope grants the full SMART allow-list (openid, offline_access, patient/*.read, user/*.read, launch/patient). Stolen registration token → broad client. |
| Introspect | Any confidential client can introspect any access/refresh (sub, patient, jti). Not token-owner scoped. |
GrantTypes unused |
DCR stores them; handlers never enforce. issueTokens always mints a refresh, including client_credentials. |
| Registration bearer compare | Not constant-time. |
FHIRAudience: issuer |
Builtin tokens get aud = AS issuer, not /fhir. Authorize does not check aud unless LaunchResolver is set (it is not on haistack serve). Inferno reference sets a launch resolver; serve does not. |
| Production always enables DCR | Config comment says “token or disabled DCR”; runtime always sets AllowDynamicRegistration: true. |
Postgres 0015 collision |
Renaming oauth → 0017 is correct for new DBs. A DB that already recorded version 15 from old 0015_oauth.sql will still skip terminology 0015. In-flight refresh rows get issuer='' and become unconsumable (no backfill). |
| Clients / replay / revocation / rate-limit tables | Not issuer-scoped. Shared SQL/Redis is OK only because consume is issuer-bound and jti is 192-bit. |
| Docs drift | PR body still mentions DefaultProductionPaths (renamed to DefaultSigningKeyPaths). pkg/runtime/README.md still says auth middleware is not wired by default. |
Pre-existing (touched, not introduced)
Worth tracking if this is the production OAuth PR, but they were already on main:
?patient=is copied into the JWT and becomesTenant.PatientScopeviamergeLaunch. No check that the user may access that patient. Query param wins over launch context.- Token success responses omit
Cache-Control: no-store(writeJSON). - Refresh rotation has no family reuse detection; auth-code reuse does not revoke already-issued tokens.
- Access-token revoke uses
ParseTokenUnverified(forgedjti+ matchingiss/client_idcan denylist). randomTokenfalls back toUnixNanoifrand.Readfails.- PKCE method is not rejected at authorize (
plainfails later at token, which is correct, but the code is still issued).
What looks solid
- Issuer binding is real:
RequireBoundIssueron save; consume/lookup take issuer; composite PKs(issuer, code\|token\|id); Redis keysbase64url(issuer); Lua consume fail-closes on issuer/expiry mismatch and does not burn the key. Tests cover cross-issuer code/refresh/consent and a multi-tenant HTTP attempt. - Auth-code consume-once: SQL
DELETE … RETURNING; Redis atomic Lua. - DCR: HTTPS redirect URIs (loopback HTTP allowed); scope allow-list; production requires
RegistrationAccessTokenon the Bearer header only. - PKCE: required for public clients;
plainnot accepted; production setsRequirePKCEForAllClients. - Multi-tenant bearer: unverified
issonly selects a server, then verifies with that server’s key +ExpectedIssuer/ExpectedAudience. - Rate limits: fail-closed on store error; IP from
RemoteAddronly. - File stores gone from production path; SQLite/Postgres required for builtin OAuth. Breaking API is documented in the PR body (except the
DefaultProductionPathsname). - Production gates for https issuer, registration token, encryption secret, session secret,
AutoApprove=falseare actually enforced in config + runtime.
Test / CI gaps
- CI:
make fmtonpkg/oauth/security_test.go. - No test that
/fhir/Patientwithout bearer is 401 with builtin OAuth, or that/fhir/Patient/metadatais not public. - No rotation test (old kid still in JWKS / still verifies).
- No encrypted signing-key round-trip through the table.
- SQL tests barely cover refresh consume / pending sessions (auth-code + replay only). Persist test uses file PEM, not
hai_oauth_signing_key. - No migrate-from-main fixture for a DB that already applied
0015_oauth. - Runtime tests hit discovery/JWKS/tenant well-known and production failure cases, not a full authorize → token → FHIR read through
WithBuiltinOAuth.
Suggested merge bar
- Fix gofmt.
- Remove or gate passwordless production login; drop
haistack-appfrom production; allow-listreturn. - Fix rotation + verify with
VerificationKeys. - Fix SQLite expiry comparison.
- Add the missing negative auth and rotation tests.
Issuer binding and the store consolidation can stay; they are the right model. The production session/login and rotation paths need another pass before this should be described as production OAuth.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if returnURL == "" { | ||
| returnURL = s.cfg.Issuer + "/oauth/authorize" | ||
| } | ||
| http.Redirect(w, r, returnURL, http.StatusFound) |
There was a problem hiding this comment.
Production login issues a signed session for whatever username is posted. There is no password, user directory, or IdP check.
WithBuiltinOAuth installs this authenticator whenever oauth.production is true. Anyone who can reach /oauth/login can mint a 12h cookie as an arbitrary sub and complete /oauth/authorize.
This cannot be the production user-login path. Keep UserAuthenticator as the seam and plug in a real identity source.
|
|
||
| func isPublicFHIRPath(path string) bool { | ||
| trimmed := strings.TrimSuffix(strings.TrimSpace(path), "/") | ||
| return strings.HasSuffix(trimmed, "/metadata") |
There was a problem hiding this comment.
This is a suffix match, not “the metadata route.” /fhir/Patient/metadata (legal FHIR id) skips bearer middleware.
Today instance authorize still fail-closes (identityFromContext missing → unauthenticated) when AuthChecker is set, so this is not a data leak on the builtin path. It is still the wrong predicate: match {BasePath}/metadata exactly, and add a test that /fhir/Patient/metadata requires a bearer.
| // RegisterRateLimiter overrides the default in-memory registration limiter. | ||
| RegisterRateLimiter RateLimitStore | ||
| // VerificationKeys are additional public keys exposed via JWKS (for rotation). | ||
| VerificationKeys []*KeySet |
There was a problem hiding this comment.
JWKS merges these with SigningKey, but BearerAuthConfig and introspectAccessToken still verify only cfg.SigningKey.
After a correct rotation (previous kid kept here for the access-token TTL), external resource servers using JWKS may accept tokens this process will 401. Build the verifier from active + verification keys.
| err := s.db.QueryRowContext(context.Background(), ` | ||
| DELETE FROM hai_oauth_auth_code | ||
| WHERE code = ? AND expires_at > ? AND issuer = ? | ||
| RETURNING payload`, code, now, issuer, |
There was a problem hiding this comment.
SQLite expiry is a string compare of time.RFC3339Nano, which strips trailing fractional zeros. Lexicographic order is not time order:
- stored
…T12:00:00Zvs now…T12:00:00.5Z→'Z' > '.'→ still treated as unexpired
Postgres uses timestamptz and is not affected. Use unix seconds (or the existing SQLite time helpers) for expires_at comparisons in consume/lookup/revocation.
| if _, err := tx.ExecContext(context.Background(), ` | ||
| UPDATE hai_oauth_signing_key | ||
| SET active = 0, retired_at = ? | ||
| WHERE issuer = ? AND active = 1 AND retired_at = ''`, now, issuer); err != nil { |
There was a problem hiding this comment.
Rotation sets retired_at in the same UPDATE that clears active. scanSigningKeyRows then omits any row with retired_at != '' from both Active and Verification, so JWKS is only the new kid.
pkg/oauth/OPERATIONS.md says retired keys remain in JWKS until retired_at is set. OAUTH_SIGNING_KEY_ROTATE=1 therefore drops every in-flight access token (default TTL 1h) on the next start.
Leave retired_at empty until token TTL, and add a test that the previous kid still verifies after rotate.
Also: BearerAuthConfig / introspectAccessToken only build a verifier from SigningKey, not VerificationKeys. Even with the scan fix, this process would reject tokens whose keys it still publishes in JWKS.
| 4. Set `OAUTH_SESSION_SECRET` for production consent login sessions. | ||
| 5. Set `oauth.production: true` and `oauth.autoApprove: false`. | ||
| 6. Back up signing keys (DB table `hai_oauth_signing_key` or PEM at `{sqlite-dir}/oauth/oauth-signing.pem`). | ||
| 7. Optionally set `OAUTH_SIGNING_KEY_ROTATE=1` before restart to rotate the active signing key (retired keys remain in JWKS until `retired_at` is set). |
There was a problem hiding this comment.
This contradicts rotation in pkg/oauth/store/signing_key.go: rotate*SigningKey sets retired_at immediately, and load drops those rows from JWKS.
Either delay retired_at until access-token TTL, or change this sentence. As written, operators will rotate and invalidate every live access token.
| RedirectURIs: []string{"http://127.0.0.1/callback", "http://localhost/callback"}, | ||
| Scopes: []string{"openid", "offline_access", "patient/*.read", "user/*.read", "launch/patient"}, | ||
| }); err != nil { | ||
| return fmt.Errorf("runtime: oauth client: %w", err) |
There was a problem hiding this comment.
This public client is registered for every builtin OAuth process, including production.
Redirects are http://127.0.0.1/callback and http://localhost/callback. That is the SMART loopback pattern: the code is delivered to the browser machine's localhost, not the AS host.
Together with passwordless /oauth/login, an attacker can authorize haistack-app against a remote production AS and collect tokens on their own loopback.
Skip this client when cfg.Production is true (or require an explicit opt-in).
| returnURL := strings.TrimSpace(r.URL.Query().Get("return")) | ||
| if r.Method == http.MethodGet { | ||
| if _, loggedIn := s.authenticatedUser(r); loggedIn && returnURL != "" { | ||
| http.Redirect(w, r, returnURL, http.StatusFound) |
There was a problem hiding this comment.
Open redirect: both the already-logged-in GET and the POST success path redirect to the raw return query parameter with no same-origin / issuer allow-list.
Authorize itself builds Issuer + RequestURI, but this endpoint does not enforce that. /oauth/login?return=https://evil.example/ is accepted after the session cookie is set.
Allow-list return to the configured issuer origin (and prefer a relative path).
| } | ||
| refresh := randomToken() | ||
| _ = s.authStore.SaveRefreshToken(refresh, RefreshTokenEntry{ | ||
| Issuer: s.cfg.Issuer, |
There was a problem hiding this comment.
SaveRefreshToken error is ignored (_ =). Consume already deleted the previous refresh in handleRefreshToken. If this insert fails, the client gets a refresh the store never persisted and the previous refresh is gone.
Fail the token response if save fails. (The Issuer: field itself is correct.)
2 participants
OAuth port to main: built-in AS with SQLite/Postgres stores, multi-tenant
/t/{tenantId}/, issuer binding, introspection, rate limits, DB signing keys, Inferno workflow, production defaults.Breaking: Removed file-backed
NewProductionServer/ProductionStoresand JSON file stores for tokens, clients, and revocation. Useoauth/store(SQLite or Postgres) or in-memory stores for tests.DefaultProductionPathsremains for PEM signing key fallback only.