Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6422c921a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive OAuth 2.0 authentication layer, featuring JWT decoding, claim extraction, and path-based policy matching using a longest-prefix algorithm. It also introduces HMAC-SHA256 hashing for secure introspection caching and integrates the authentication context directly into the HTTP request lifecycle. Feedback focuses on improving the implementation by refactoring duplicated scope extraction logic, optimizing scope validation performance through the use of a hash set, and replacing hand-rolled Base64Url functions with OpenSSL's native library implementations for enhanced security and maintainability.
|
Due to we change the design plan, where use the jwt-cpp as the jwt library to reference it. |
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d8068f3d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces the foundational components for an authentication layer, supporting OAuth 2.0 token validation and JWT processing. It adds configuration structures for issuers and policies, logic for extracting claims and scopes, and a path-based policy matcher using longest-prefix semantics. Additionally, it implements a secure HMAC-SHA256 token hashing mechanism for caching and integrates the authentication context into the HTTP request lifecycle. Feedback focuses on improving the robustness of claim extraction by explicitly handling large unsigned integers and refactoring duplicated logic in scope extraction.
OAuth 2.0 Token Validation: Design Spec + Phase 1 Foundation (Steps 1–2) with jwt-cpp Adoption
Summary
This PR lands the reviewer-approved design spec (r1 → r5, 5 revisions) and the additive foundation — data structures, pure utilities, and vendored
jwt-cppv0.7.1 — for gateway-level OAuth 2.0 token validation.No runtime behavior change.
ProxyConfig::authis declared but not yet read;HttpRequest::authis always empty;AuthManager/JwtVerifier/ middleware wiring are deferred to follow-up PRs per the phase split in the spec.Branch:
support-oauth-2.0. 18 files changed, +5691 / −4 LoC (bulk is the vendored jwt-cpp headers, MIT, header-only).Design spec
Scope: Token-validator only (not a BFF / authorization-code-flow client). Supports multi-issuer deployments (Google, OpenAI, future own-IdP) with JWT verification and RFC 7662 introspection modes per issuer. Opt-in per proxy route via
upstreams[].proxy.authOR top-levelauth.policies[]withapplies_toprefixes.Review history (condensed)
UpstreamConfig::ProxyConfig(not a newroutes[]table); mutable-field pattern forreq.auth; outbound header injection inHeaderRewriter; HS256 dropped from v1ProxyConfig::operator==excludesauthwith same-PRAuthManager::Reloadwiring rule;shared_ptr<const AuthForwardConfig>snapshot model; programmatic-route scope clarified; middleware-vs-route-lookup ordering fixedjwt-cppv0.7.1 immediately (not deferred); delete Phase 1's from-scratchjwt_decode.{h,cc}in the same PRbase.h+ defineJWT_DISABLE_PICOJSON; §9 item 16 exception containment; rewrite alg:none rationale as property of OUR code (not the library)AuthContextsnippet aligned with real header; §9 item 16parse_jwksthrow maps to undetermined/503, notinvalid_tokenKey design decisions — top-of-mind (Appendix B has 30 rows total)
UpstreamManagerfor IdP calls (inherits TLS, pool, CB, retry, metrics)auth.policies[]withapplies_toalg: nonedefenseJwtVerifier::BuildVerifier()never callsallow_algorithm(jwt::algorithm::none{}); ConfigLoader rejects"none"at load; mandatory test asserts rejectiontry/catch(std::exception)→ translated to result types (401 invalid_request / 401 invalid_token / 503 undetermined)AuthForwardConfiglifetimeshared_ptr<const>snapshot, per-call toRewriteRequest— reload-safeproxy.authreloadProxyConfig::operator==excludesauthANDAuthManager::Reloadwiring in same PR (mirrorsUpstreamConfig::operator==/CircuitBreakerManager::Reloaddiscipline)on_undetermined: "allow"opt-outIndustry comparison
§2 of the spec anchors every design choice against Nginx (OSS + Plus), Envoy (
jwt_authn/ext_authz/oauth2), Kong (jwt/oauth2/openid-connect), Traefik (ForwardAuth), andoauth2-proxy. Consensus: microservice-focused gateways split token validation from authorization-code flow into separate modules, and all of them use opt-in per-route protection for stateless API tokens.Changes in this PR
Step 1 — Data structures (additive)
New headers:
include/auth/auth_context.h—AuthContextstruct (issuer, subject, scopes, claims, policy_name, SENSITIVEraw_tokenwith "never log" contract, undetermined flag).include/auth/auth_config.h— full config type hierarchy:IntrospectionConfig,IssuerConfig,AuthPolicy,AuthForwardConfig,AuthConfig, all withoperator==for reload comparison.Modified files:
include/http/http_request.h— addedmutable std::optional<auth::AuthContext> auth;(same mutable-field pattern asparams,client_ip,async_cancel_slot). Cleared inReset()for keep-alive correctness.include/config/server_config.h— addedauth::AuthPolicy authtoProxyConfigwith explicitoperator==exclusion plus rule-cite comment pointing atDEVELOPMENT_RULES.mdand design spec §11.1. Addedauth::AuthConfig authtoServerConfig.Rationale for
ProxyConfig::operator==excludingauth: same-PR discipline fromDEVELOPMENT_RULES.md("Live-reloadable config fields in restart-required equality operators — ordering matters"). Both halves (exclusion ANDAuthManager::Reloadwiring) must land together. This PR ships the exclusion; the wiring lands in the follow-up PR that introducesAuthManager.Step 2 — Pure utilities (additive)
New files:
include/auth/token_hasher.h+server/token_hasher.ccTokenHasher::Hash()— HMAC-SHA256 with 128-bit truncation, hex-encoded. Returnsstd::optional<std::string>—nullopton HMAC failure (per r3 finding:""would collide distinct failing tokens on the same cache key, a confidentiality bug).GenerateHmacKey()— OpenSSLRAND_bytes(fails-closed on RNG failure).LoadHmacKeyFromEnv()— auto-detect base64url-32-bytes-or-raw; wrapped intry/catch(std::exception)per r5 §9 item 16 —jwt::base::decodethrows on invalid input, and we fall through to raw-bytes interpretation rather than aborting server startup.include/auth/auth_policy_matcher.h+server/auth_policy_matcher.ccAppliedPolicystruct ({prefix, AuthPolicy}) +AppliedPolicyListvector.FindPolicyForPath()— longest-prefix match. Empty-prefix catch-all supported. Case-sensitive. Documented as plain prefix (not segment-safe) — e.g.,/adminmatches/administrator/foo; operators use trailing slash/admin/for segment-safe narrowing.ValidatePolicyList()— detects exact-prefix collisions with human-readable errors.include/auth/auth_claims.h+server/auth_claims.ccExtractScopes()— handlesscope(space-separated string),scp(array),scopes(Azure AD style).PopulateFromPayload()— buildsAuthContextfrom JWT payload + operator-requested claim keys; rejects payloads missingissorsub. Array/object claims are deliberately skipped at this layer (flattening is a Phase 3HeaderRewriterconcern because serialization depends on upstream expectations).HasRequiredScopes()/MatchesAudience()— enforcement helpers.jwt-cpp adoption (per r4 / r5)
Vendored —
third_party/jwt-cpp/(v0.7.1, MIT, header-only):include/jwt-cpp/jwt.h(~4228 LoC)include/jwt-cpp/base.h(~356 LoC — required because jwt.h includes it, andtoken_hashercallsjwt::base::decodedirectly)include/jwt-cpp/traits/nlohmann-json/defaults.h+traits.hLICENSE(MIT)NOT vendored:
picojson/picojson.h— suppressed via-DJWT_DISABLE_PICOJSONmacro. We use nlohmann/json.Makefile changes:
-Ithird_party/jwt-cpp/includeand-DJWT_DISABLE_PICOJSONtoCXXFLAGS.JWT_CPP_DIRmacro + 4 jwt-cpp headers added toAUTH_HEADERSfor dependency tracking (so a bump-jwt-cpp PR correctly invalidates the whole build).Deleted files (superseded by jwt-cpp):
include/auth/jwt_decode.hserver/jwt_decode.ccBase64UrlEncode/Decode,JwtDecoded, andDecode(). All superseded byjwt::decode<traits>(token)+jwt::base::{encode,decode}<jwt::alphabet::base64url>(). Preserves the strict-padding base64url fix from the earlier code review — jwt-cpp's decoder is also strict by construction.