Review fixes

[ScriptSmith]

No description provided.

[greptile-apps]

Greptile Summary

This PR addresses the two previously flagged security issues (OIDC JWKS DNS rebinding, bootstrap auth shared "unknown" IP lockout bucket) and adds a broad set of related hardening: per-fetch DNS pinning for JWKS and discovery URLs, reserved-role stripping from IdP claims and proxy headers, tenant-scoped cache keys for response and semantic caches, bootstrap brute-force rate limiting, and proxy auth failing closed when trusted_proxies is unconfigured. The breaking change to proxy auth (no-longer trusting headers when trusted_proxies is absent) is gated by a startup config validation error, so existing deployments will see an explicit failure rather than silent behavior change.

Confidence Score: 5/5

Safe to merge; both previously flagged P1/P0 issues are resolved and only P2 observations remain.

All findings are P2 (minor information leak and a partial DNS-pinning gap for non-JWKS OIDC endpoints). No new P0/P1 issues introduced. The breaking proxy-auth change is guarded by a startup validation error. Test coverage for the new paths is thorough.

src/auth/oidc.rs — token/userinfo endpoint calls still use unpinned http_client (P2). src/middleware/layers/admin.rs — bootstrap key length oracle via length-based early return (P2, documented trade-off).

Important Files Changed

Filename	Overview
src/auth/jwt.rs	Replaced shared reqwest client with per-fetch pinned client in refresh_jwks; added with_options constructor storing UrlValidationOptions; added audience-empty-string validation to check_config. Fully closes JWKS DNS-rebinding window.
src/auth/gateway_jwt.rs	Added per-IP rate limiting on JWT lazy-loads, refactored negative cache to use VecDeque for O(1) LRU eviction, switched to JwtValidator::with_options for DNS pinning. Logic is sound and well-tested.
src/auth/oidc.rs	Added SSRF validation + DNS pinning for discovery URL, issuer mismatch check, endpoint SSRF validation at discovery time, and reserved-role stripping. Token/userinfo endpoint calls still use unpinned http_client (P2 gap noted).
src/middleware/layers/admin.rs	Added strip_reserved_roles, bootstrap rate-limit/lockout, fixed "unknown" IP shared bucket (previous P1 resolved), hardened proxy auth to fail closed. Bootstrap key length oracle is a minor P2 trade-off.
src/cache/keys.rs	Added CacheTenantScope struct mixed into all cache key hash functions; added bootstrap rate-limit/lockout key helpers. Tenant isolation is comprehensive and tested.
src/cache/vector_store/pgvector.rs	Rewrote search query builder to support dynamic tenant filters with NULL-matching for unscoped requests; uses placeholder substitution to build parameterized queries safely. Logic appears correct.
src/cache/vector_store/qdrant.rs	Added server-side org/project filter for scoped requests; acknowledged known limitation for unscoped queries (top_k exhaustion miss) with post-filter safety net in SemanticCache. Matches previously flagged P2 behavior.
src/validation/url.rs	Added pinned_reqwest_client using reqwest's resolve_to_addrs to pin DNS resolution to validated addresses; wasm32 stub correctly defers to browser security boundary.
src/config/mod.rs	Config validation now refuses startup for IAP mode without trusted_proxies (removed the localhost loopback exception); aligns with proxy auth middleware failing closed.
src/config/auth.rs	Added configurable auth_state_ttl_secs to SessionConfig, SameSite::None+insecure session validation, and JWT audience empty-string validation. All changes are well-guarded.
src/cache/semantic_cache.rs	Added VectorTenantFilter to both lookup and store paths; double-enforced via server-side filter and post-filter in Step 4, ensuring cross-tenant leakage is impossible regardless of backend.

Sequence Diagram

sequenceDiagram
    participant Client
    participant AdminMiddleware
    participant OidcAuthenticator
    participant GatewayJwtRegistry
    participant JwtValidator
    participant Cache
    participant DB

    Client->>AdminMiddleware: Request with Bearer token
    AdminMiddleware->>AdminMiddleware: strip_reserved_roles(claims.roles)
    AdminMiddleware->>GatewayJwtRegistry: find_or_load_by_issuer(issuer, rate_limit)
    GatewayJwtRegistry->>Cache: check_and_incr_rate_limit(gw:jwt:lazy_load:{ip})
    Cache-->>GatewayJwtRegistry: allowed/denied
    GatewayJwtRegistry->>DB: find_enabled_oidc_by_issuer
    DB-->>GatewayJwtRegistry: SSO configs
    GatewayJwtRegistry->>JwtValidator: with_options(config, UrlValidationOptions)
    JwtValidator->>JwtValidator: refresh_jwks()
    JwtValidator->>JwtValidator: validate_base_url_opts(jwks_url)
    JwtValidator->>JwtValidator: pinned_reqwest_client(validated)
    JwtValidator-->>AdminMiddleware: validated claims

    Client->>OidcAuthenticator: OIDC callback
    OidcAuthenticator->>OidcAuthenticator: validate_base_url_opts(discovery_url)
    OidcAuthenticator->>OidcAuthenticator: pinned_reqwest_client - fetch discovery doc
    OidcAuthenticator->>OidcAuthenticator: check issuer == config.issuer
    OidcAuthenticator->>OidcAuthenticator: validate_base_url_opts(token/jwks/userinfo)
    OidcAuthenticator->>OidcAuthenticator: strip_reserved_roles(claims.roles/groups)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/auth/oidc.rs
Line: 553-573

Comment:
**Token/userinfo endpoints validated but not pinned on use**

SSRF validation for `authorization_endpoint`, `token_endpoint`, and `userinfo_endpoint` runs at discovery time, but subsequent calls to those endpoints (token exchange, userinfo fetch) still use the unpinned `self.http_client`. The `jwks_uri` case is fully fixed by the new per-fetch pinned client in `JwtValidator::refresh_jwks`, but a short-TTL DNS rebind between discovery and the token-exchange POST could redirect an auth-code delivery to an internal address. The impact is narrower than the JWKS case (the received tokens would still fail JWKS validation), but the gateway would be making an outbound POST to an attacker-controlled internal target.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/middleware/layers/admin.rs
Line: 745-748

Comment:
**Bootstrap key length exposed via length-based early return**

`could_be_bootstrap_guess` only returns `Ok(None)` (without touching the rate-limit counter) for keys whose length differs from the bootstrap key. An attacker can probe all key lengths: requests with wrong-length keys produce no lockout, while the matching length eventually triggers it. This makes the bootstrap key length trivially enumerable. The code comment acknowledges the trade-off (normal JWT bearer tokens must not exhaust the rate limit), but the information leak is worth noting for operators: bootstrap key length becomes observable through lockout behavior.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (7): Last reviewed commit: "Fix WebSocket race condition" | Re-trigger Greptile}

[ScriptSmith]

@greptile-apps