diff --git a/.claude/rules/authentication_authorization.md b/.claude/rules/authentication_authorization.md index 6185dbac1..7c82b919e 100644 --- a/.claude/rules/authentication_authorization.md +++ b/.claude/rules/authentication_authorization.md @@ -335,10 +335,8 @@ Broken access control on admin and internal endpoints is one of the most severe ### Non-Compliant Code ```go -// ERROR: Assumes this handler is only reachable internally / by admins because -// it's registered under an "/internal" or "/admin" path prefix — no scope check -// inside the handler itself. A JWT bypass, misrouted proxy, or shared Key Manager -// misconfiguration upstream is enough to reach this code with any valid token. +// ERROR: Assumes network placement/path prefix is sufficient protection — no +// scope check inside the handler itself. func RotateApiKeyHandler(w http.ResponseWriter, r *http.Request) { apiID := r.URL.Query().Get("api_id") newKey, err := rotateApiKey(apiID) @@ -434,7 +432,7 @@ Authenticated SQL injection in Admin REST APIs is an exploitable bug class: an a func SearchTenantsHandler(w http.ResponseWriter, r *http.Request) { name := r.URL.Query().Get("name") query := "SELECT id, name, status FROM tenants WHERE name LIKE '%" + name + "%'" - rows, err := db.Query(query) // name = "%' OR '1'='1" defeats the filter entirely + rows, err := db.Query(query) // Injectable — filter can be defeated if err != nil { http.Error(w, "Internal Error", http.StatusInternalServerError) return @@ -664,8 +662,7 @@ Open redirect is a persistently recurring vulnerability class. Weak callback URL ```go // ERROR: Redirects to whatever the caller supplies, with only a same-host -// substring check — bypassable via "https://trusted.example.com.attacker.com" -// or an open second-parameter trick like "https://trusted.example.com@attacker.com". +// substring check — bypassable via crafted lookalike/userinfo hosts. func LoginCallbackHandler(w http.ResponseWriter, r *http.Request) { returnTo := r.URL.Query().Get("returnTo") if strings.Contains(returnTo, "trusted.example.com") { @@ -727,4 +724,685 @@ func LoginCallbackHandler(w http.ResponseWriter, r *http.Request) { > **Verification Checklist before outputting code:** > * Is a redirect target validated with a substring/prefix check (`strings.Contains`, `strings.HasPrefix`) rather than a parsed-URL host comparison against an explicit allowlist? (Substring checks are bypassable — replace with `url.Parse` + exact host match.) > * Does the validated redirect target allow scheme-relative URLs (`//attacker.com`) or a userinfo trick (`https://trusted.com@attacker.com`)? (Both parse as "same host" under naive checks — `safeRedirectTarget`-style explicit host comparison closes both.) -> * On rejection, does the handler redirect to a safe default rather than erroring in a way that reflects the rejected URL back into the response? (Reflecting it back risks reintroducing an XSS surface — see `js-output-encoding-xss.md` for the same principle in JS.) \ No newline at end of file +> * On rejection, does the handler redirect to a safe default rather than erroring in a way that reflects the rejected URL back into the response? (Reflecting it back risks reintroducing an XSS surface — see `js-output-encoding-xss.md` for the same principle in JS.) + +--- + +## GO-AUTH-011: Fail-Closed Startup Validation for Security-Critical Configuration + +### Severity + +Critical + +### Description + +At startup, validate that the *resulting* security configuration is actually enforceable, not merely that each field in isolation is well-formed. If authentication is nominally "enabled" but no authenticator can actually be constructed from the given config (e.g. basic-auth enabled with zero registered users), or a control-plane channel is configured to run without TLS, the process must refuse to start — or, in an explicitly-flagged development mode only, fail loudly on every request — rather than silently falling back to an open/passthrough state. + +### Rationale + +Per-field validation is not the same as validating the resulting behavior. A config can look superficially valid ("auth is enabled") while still producing zero effective authenticators, silently degrading to an open/passthrough state that a check scoped only to top-level flags wouldn't catch. + +### Non-Compliant Code + +```go +// ERROR: registers an authenticator only if BasicAuth.Enabled && len(Users) > 0. +// If Enabled is true but Users is empty, zero authenticators are registered — +// and the code silently treats that as "no auth needed" rather than "misconfigured." +func BuildAuthenticators(cfg AuthConfig) []Authenticator { + var authns []Authenticator + if cfg.BasicAuth.Enabled && len(cfg.BasicAuth.Users) > 0 { + authns = append(authns, NewBasicAuthenticator(cfg.BasicAuth.Users)) + } + return authns // Can be empty even though BasicAuth.Enabled == true +} + +func AuthMiddleware(authns []Authenticator) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if len(authns) == 0 { + // ERROR: passthrough — every request is treated as authenticated. + ctx := context.WithValue(r.Context(), AuthContextKey, AuthContext{ + Authenticated: true, UserID: "sys_noauth_user", + }) + ctx = context.WithValue(ctx, AuthzSkipKey, true) + next.ServeHTTP(w, r.WithContext(ctx)) + return + } + // ... real check + }) + } +} +``` + +### Compliant Code + +```go +// CORRECT: startup validation checks the *effective* result of the config, +// not just each field — refuses to start rather than silently degrading. +// NOTE: GO-AUTH-012 adds a third check (IDP expected-audience) to this SAME +// function rather than defining a second, differently-bodied +// ValidateAuthConfig — see the compliant example there for the merged form. +func ValidateAuthConfig(cfg AuthConfig) error { + if cfg.Disable { + // Explicit, off-by-default opt-out (see AuthConfig below) — startup is + // allowed with zero configured authenticators; AuthMiddleware is what + // logs the banner-level warning on every request while this is set. + return nil + } + if cfg.BasicAuth.Enabled && len(cfg.BasicAuth.Users) == 0 { + return fmt.Errorf("basic auth is enabled but no users are configured — " + + "refusing to start with an unenforceable auth config") + } + if !cfg.BasicAuth.Enabled && !cfg.IDP.Enabled { + return fmt.Errorf("no authentication method is configured — " + + "set auth.disable=true explicitly if this is intentional") + } + return nil +} + +func main() { + cfg := loadConfig() + if err := ValidateAuthConfig(cfg.Auth); err != nil { + log.Fatalf("fatal: invalid auth configuration: %v", err) // Refuse to start + } + // ... +} + +// An explicit, off-by-default opt-out — never an implicit fallback. +type AuthConfig struct { + Disable bool // Must be explicitly set; logs a startup banner-level warning on every request when true + BasicAuth BasicAuthConfig + IDP IDPConfig +} +``` + +> **Verification Checklist before outputting code:** +> * Can any combination of "enabled" flags and empty sub-config (e.g. an empty `users` list) result in zero authenticators being registered while auth is still nominally "enabled"? (If yes, add a startup check that fails closed on this combination specifically, not just on `Enabled == false`.) +> * Does the existing "no authentication configured" warning check the *effective* authenticator count, or only whether specific top-level flags are false? (A flag-only check misses the "enabled but unusable" case.) +> * Is there an explicit, off-by-default `disable`/`insecure` flag for intentionally running without auth, distinct from a configuration that merely fails to produce a working authenticator? + +--- + +## GO-AUTH-012: JWT Audience and Issuer Must Be Explicitly Validated + +### Severity + +High + +### Description + +Every JWT verification path must set an expected audience (`aud`) and issuer (`iss`) and reject tokens that omit or mismatch them, enforced at the parsing-library level (`jwt.WithAudience`, `jwt.WithIssuer`) rather than trusted as claims to be read and compared manually (or not compared at all). Configuration that enables IDP-based JWT auth must require a non-empty expected audience before the server will start. + +### Rationale + +A token correctly signed by a trusted IDP but *minted for a different client or service* (a different audience) still passes signature and expiry checks. Without an audience check, any valid token from the same IDP — regardless of which application it was issued for — is accepted as a credential for this service, collapsing the boundary between "this IDP is trusted" and "this specific token was intended for me." + +### Non-Compliant Code + +```go +// ERROR: parses and validates signature/expiry, but never checks `aud` against +// this service's expected audience — any token from the trusted IDP is accepted +// regardless of which client/service it was actually issued for. +func VerifyManagementToken(tokenStr string, idpPublicKey interface{}) (*Claims, error) { + claims := &Claims{} + _, err := jwt.ParseWithClaims(tokenStr, claims, func(t *jwt.Token) (interface{}, error) { + return idpPublicKey, nil + }) + if err != nil { + return nil, err + } + return claims, nil // claims.Audience is never inspected +} +``` + +### Compliant Code + +```go +// CORRECT: expected audience is required configuration, enforced by the +// library itself so the check cannot be silently skipped by a future refactor. +// This check is added to the SAME ValidateAuthConfig defined under GO-AUTH-011 +// — not a second, redefined function — so both checks run from one call site. +type IDPConfig struct { + JWKSURL string + Issuer string + ExpectedAudience string // Required whenever IDP.Enabled == true +} + +func ValidateAuthConfig(cfg AuthConfig) error { + if cfg.Disable { // From GO-AUTH-011 — explicit, off-by-default opt-out + return nil + } + if cfg.BasicAuth.Enabled && len(cfg.BasicAuth.Users) == 0 { // From GO-AUTH-011 + return fmt.Errorf("basic auth is enabled but no users are configured — " + + "refusing to start with an unenforceable auth config") + } + if !cfg.BasicAuth.Enabled && !cfg.IDP.Enabled { // From GO-AUTH-011 + return fmt.Errorf("no authentication method is configured — " + + "set auth.disable=true explicitly if this is intentional") + } + if cfg.IDP.Enabled && cfg.IDP.ExpectedAudience == "" { // Added by GO-AUTH-012 + return fmt.Errorf("idp auth is enabled but no expected audience is configured — " + + "refusing to start with an unenforceable token scope") + } + return nil +} + +func VerifyManagementToken(tokenStr string, idpPublicKey interface{}, cfg IDPConfig) (*Claims, error) { + claims := &Claims{} + _, err := jwt.ParseWithClaims(tokenStr, claims, func(t *jwt.Token) (interface{}, error) { + // GO-AUTH-002: restrict to asymmetric algorithms before ever returning + // the key — never let the token header alone decide how it's verified, + // and never accept "none" or a symmetric (HS*) algorithm here. The + // key-type check below additionally guards against a token whose alg + // claims one asymmetric family while idpPublicKey is actually another. + switch t.Method.(type) { + case *jwt.SigningMethodRSA: + if _, ok := idpPublicKey.(*rsa.PublicKey); !ok { + return nil, fmt.Errorf("configured key does not match RSA signing method") + } + case *jwt.SigningMethodECDSA: + if _, ok := idpPublicKey.(*ecdsa.PublicKey); !ok { + return nil, fmt.Errorf("configured key does not match ECDSA signing method") + } + case *jwt.SigningMethodEd25519: + if _, ok := idpPublicKey.(ed25519.PublicKey); !ok { + return nil, fmt.Errorf("configured key does not match Ed25519 signing method") + } + default: + return nil, fmt.Errorf("unexpected or forbidden signing method: %v", t.Header["alg"]) + } + return idpPublicKey, nil + }, + jwt.WithValidMethods([]string{"RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "EdDSA"}), // Explicit allowlist, defense-in-depth with the switch above + jwt.WithAudience(cfg.ExpectedAudience), // Library-enforced — fails closed on mismatch + jwt.WithIssuer(cfg.Issuer), + ) + if err != nil { + return nil, fmt.Errorf("token verification failed: %w", err) + } + return claims, nil +} +``` + +> **Verification Checklist before outputting code:** +> * Does every `jwt.ParseWithClaims`/`jwt.Parse` call for a management or admin-facing token pass `jwt.WithAudience(...)`? (If the `aud` claim exists on the struct but is never checked via a parser option, add it.) +> * Is the expected audience sourced from required configuration that fails startup validation when IDP auth is enabled but the audience is empty? (If the audience is optional or defaulted silently, make it mandatory per GO-AUTH-011's pattern.) +> * Is `iss` (issuer) validated the same way? (Add `jwt.WithIssuer` alongside the audience check.) +> * Does this audience check live inside the same `ValidateAuthConfig` function established by GO-AUTH-011, rather than a second function of the same name/signature? (A duplicate definition is a compile error, and whichever copy survives silently drops the other rule's check.) + +--- + +## GO-AUTH-013: Admin, Debug, and Metrics Interfaces Require Real Authentication, Not Merely IP Allowlisting + +### Severity + +Critical + +### Description + +Any admin, debug, or metrics HTTP/gRPC endpoint that exposes configuration dumps, runtime state, or sensitive operational data must be protected by the same authentication/authorization stack used for regular management APIs (GO-AUTH-007). An IP allowlist is defense-in-depth only — it must never default to allow-all (`["*"]`), and `Validate()` must reject a wildcard allowlist unless an explicit, off-by-default administrative flag opts into it. + +### Rationale + +An admin debug endpoint (`config_dump`) or a metrics endpoint reachable with no authentication, gated only by an IP allowlist that defaults to `["*"]`, is functionally unauthenticated from day one of any deployment that didn't override the default. IP-based controls are also routinely defeated inside a Kubernetes cluster (pod-to-pod traffic, a compromised sidecar) where "the caller's IP looks internal" is not evidence the caller is authorized. + +### Non-Compliant Code + +```go +// ERROR: config_dump is reachable by anyone whose IP passes an allowlist that +// defaults to allow-all — no authentication is applied at all. +func defaultConfig() AdminConfig { + return AdminConfig{ + AllowedIPs: []string{"*"}, // Allow-all by default + } +} + +func (s *AdminServer) configDumpHandler(w http.ResponseWriter, r *http.Request) { + if !isAllowedIP(r.RemoteAddr, s.cfg.AllowedIPs) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + json.NewEncoder(w).Encode(s.currentConfig) // No authentication check at all +} +``` + +### Compliant Code + +```go +// CORRECT: real authentication (the same middleware protecting the management +// API) gates the admin/debug/metrics mux, and the IP allowlist — still present +// as defense-in-depth — defaults to loopback-only. +func defaultConfig() AdminConfig { + return AdminConfig{ + AllowedIPs: []string{"127.0.0.1", "::1"}, // Loopback-only default + } +} + +func (c AdminConfig) Validate() error { + for _, ip := range c.AllowedIPs { + if ip == "*" && !c.AllowWildcardIPAllowlist { + return fmt.Errorf("admin.allowed_ips=[\"*\"] requires the explicit " + + "admin.allow_wildcard_ip_allowlist opt-in flag") + } + } + return nil +} + +// NewAdminMux reuses the exact RequireScope (GO-AUTH-007) and AuthMiddleware +// (GO-AUTH-011) helpers already defined in this file — never bespoke, +// differently-signed helpers invented per rule. +func NewAdminMux(cfg AdminConfig, authns []Authenticator) http.Handler { + mux := http.NewServeMux() + mux.Handle("/config_dump", RequireScope("admin:config:read", http.HandlerFunc(configDumpHandler))) + mux.Handle("/xds_sync_status", RequireScope("admin:xds:read", http.HandlerFunc(xdsSyncStatusHandler))) + + protected := AuthMiddleware(authns)(mux) // Real authentication — GO-AUTH-011 + return ipAllowlistMiddleware(cfg.AllowedIPs)(protected) // IP check as defense-in-depth, not the only check +} +``` + +> **Verification Checklist before outputting code:** +> * Does an admin/debug/metrics handler rely solely on an IP allowlist with no authentication middleware in front of it? (If yes, wrap it in the same `AuthMiddleware`/`RequireScope` stack as management API routes.) +> * Does the default `AllowedIPs` value include `"*"` or an equivalent allow-all wildcard? (If yes, change the default to loopback-only and require an explicit opt-in flag to widen it.) +> * Do metrics labels expose high-cardinality, tenant-shaped values (org IDs, API IDs) to an unauthenticated scrape endpoint? (If yes, gate the endpoint the same way, and/or bucket/anonymize the label values.) + +--- + +## GO-AUTH-014: No Default, Hardcoded, or Shipped Credentials + +### Severity + +Critical + +### Description + +Packaged/shipped configuration must never contain a functional default username/password (e.g. `admin`/`admin`). Either ship with zero users configured — which, combined with GO-AUTH-011's fail-closed startup validation, forces an operator to provide credentials before the service will run — or generate a random initial credential on first boot, persisted to a file with restrictive permissions, and never leave a static default credential active in a production build. + +### Rationale + +A shipped `config.toml` containing `username = "admin"` / `password = "admin"` is a credential every installer of the product possesses before they've configured anything; if it is ever left unchanged (a near-certainty across a large install base), it is a direct authentication bypass indistinguishable from no authentication at all. + +### Non-Compliant Code + +```toml +# ERROR: functional default credentials shipped in packaged configuration — +# present in every fresh install until an operator remembers to change them. +[controller.auth.basic] +enabled = true +[[controller.auth.basic.users]] +username = "admin" +password = "admin" +password_hashed = false +roles = ["admin"] +``` + +### Compliant Code + +```go +// CORRECT: on first boot with zero configured users, generate a random +// credential, persist its hash restrictively, and never leave a static +// default active in a production build. Crucially, the persisted HASH is +// reloaded into cfg.Users on every subsequent boot — not just written once — +// so a restart doesn't leave BasicAuth.Enabled with zero users (which would +// otherwise trip GO-AUTH-011's own fail-closed startup check on the very +// next boot). +func EnsureInitialAdminCredential(cfg *BasicAuthConfig, dataDir string) error { + if len(cfg.Users) > 0 { + return nil // Operator has already configured credentials + } + + credentialFile := filepath.Join(dataDir, "initial-admin-credential.hash") + + if existingHash, err := os.ReadFile(credentialFile); err == nil { + // Already generated on a previous boot — reload the persisted hash + // rather than leaving cfg.Users empty. + cfg.Users = []BasicAuthUser{{Username: "admin", PasswordHash: string(existingHash), Roles: []string{"admin"}}} + return nil + } else if !os.IsNotExist(err) { + return fmt.Errorf("failed to read persisted admin credential: %w", err) + } + + randomPassword, err := generateSecurePassword(24) // crypto/rand-backed + if err != nil { + return fmt.Errorf("failed to generate initial admin credential: %w", err) + } + + hashed, err := bcrypt.GenerateFromPassword([]byte(randomPassword), bcrypt.DefaultCost) + if err != nil { + return fmt.Errorf("failed to hash initial admin credential: %w", err) + } + if err := os.WriteFile(credentialFile, hashed, 0o600); err != nil { + return fmt.Errorf("failed to persist initial admin credential: %w", err) + } + + // The plaintext password is shown to the operator exactly once, at + // generation time, and never written to disk or logs — only the bcrypt + // hash above is persisted. + fmt.Fprintf(os.Stderr, "Generated one-time initial admin password: %s — rotate it after first login.\n", randomPassword) + cfg.Users = []BasicAuthUser{{Username: "admin", PasswordHash: string(hashed), Roles: []string{"admin"}}} + return nil +} +``` + +> **Verification Checklist before outputting code:** +> * Does any packaged/shipped config file, Helm chart default, or Dockerfile set a functional (non-placeholder) username and password for an admin or service account? (If yes, remove it — see the two compliant patterns above.) +> * If a "development mode" flag gates a default credential, is that flag off by default and does the code log a loud, unambiguous warning whenever it is on? (A quiet or easily-forgotten dev flag is equivalent to a shipped default in practice.) +> * Is a generated initial credential's hash written with `0o600` (or equivalent) permissions and never logged in plaintext to a shared log stream? (Only the one-time console output should show the plaintext; persisted storage and logs must hold the hash only, per GO-AUTH-003.) +> * On a restart after the credential was already generated, is the persisted hash reloaded into `cfg.Users`, rather than merely detected-and-skipped? (If the function returns early without repopulating `cfg.Users`, GO-AUTH-011's fail-closed check will refuse to start the service on every boot after the first.) + +--- + +## GO-AUTH-015: Enforce Security Invariants at the Service/Data Layer, Not Only in Route Middleware + +### Severity + +High + +### Description + +A cross-cutting invariant that must hold regardless of entry point — e.g. "no mutations while the system is in immutable/maintenance mode" — must be enforced at the lowest common layer every entry point funnels through (typically the service or data-access layer), not solely in HTTP middleware scoped to one route prefix. New entry points (event-driven mutation handlers, an admin server, a future RPC surface) automatically inherit the protection only if it lives below all of them; middleware scoped to `/api/management/v0.9/*` protects only requests that happen to arrive through that specific router. + +### Rationale + +A security invariant enforced only in middleware in front of one entry point protects just the requests that traverse that specific code path. Any other entry point that reaches the same underlying operation through a different route bypasses the check entirely, because the check was never present at the point where all paths actually converge. + +### Non-Compliant Code + +```go +// ERROR: the invariant is checked only in middleware wrapping one specific +// router. Any mutation path that doesn't traverse this exact middleware chain +// (an event handler, the admin server, a future gRPC endpoint) bypasses it. +func ImmutableModeMiddleware(isImmutable func() bool) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if isImmutable() && r.Method != http.MethodGet { + http.Error(w, "Locked", http.StatusMethodNotAllowed) + return + } + next.ServeHTTP(w, r) + }) + } +} +mux.Handle("/api/management/v0.9/", ImmutableModeMiddleware(cfg.IsImmutable)(managementRouter)) +// Platform-API event handler and admin server call restAPIService.Update directly — +// neither passes through this middleware. +``` + +### Compliant Code + +```go +// CORRECT: the check lives inside the service method itself — the one place +// every caller (REST, WSS/Platform-API events, admin server, future RPC) +// necessarily passes through before a mutation can occur. +type RestAPIService struct { + db *sql.DB + isImmutable func() bool +} + +func (s *RestAPIService) Update(ctx context.Context, apiID string, def APIDefinition) error { + if s.isImmutable() { + return ErrImmutableModeActive // One check protects every entry point + } + return s.db.UpdateAPI(ctx, apiID, def) +} + +// Middleware may still exist for a fast HTTP-layer rejection (better UX/latency), +// but it is defense-in-depth on top of the service-layer check, never a substitute for it. +``` + +> **Verification Checklist before outputting code:** +> * Is a security invariant (immutable mode, a maintenance lock, a tenant-suspension check) implemented only as route middleware scoped to one router or path prefix? (If yes, move the authoritative check into the service/data layer method(s) that perform the actual mutation.) +> * Are there multiple ways to reach the same mutation (HTTP handler, event/webhook consumer, admin server, background job)? (Enumerate them and confirm the invariant check is reachable from every one, not just the primary HTTP path.) + +--- + +## GO-AUTH-016: Verify Server/Peer-Asserted Identity Against Locally-Known Configuration + +### Severity + +Medium + +### Description + +When a remote peer (a control plane, an upstream gateway) asserts its own identity or a configuration value back to you over an already-established channel (e.g. an `ack` message echoing a `gateway_id`), compare that asserted value against what you independently expect or hold locally before trusting it. An authenticated transport proves who signed the connection; it does not prove that every value inside the payload is self-consistent with what you expect — a compromised or misconfigured peer can still assert an unexpected value over a channel it legitimately authenticated to. + +### Rationale + +Accepting a server-asserted identity value without comparing it to the locally configured value lets a hostile or misconfigured peer assign an unexpected identity that the gateway then silently adopts — an identity-confusion condition between what the operator configured and what the gateway believes about itself. + +### Non-Compliant Code + +```go +// ERROR: accepts the server-asserted gateway_id with no comparison to the +// locally configured value — the connection being authenticated says nothing +// about whether this specific field is what was expected. +func (c *ControlPlaneClient) handleConnectionAck(ack *ConnectionAck) { + c.assignedGatewayID = ack.GatewayID // Trusted without verification +} +``` + +### Compliant Code + +```go +// CORRECT: the server-asserted value is checked against the locally configured +// identity before being accepted; a mismatch is treated as a security-relevant +// event and fails closed on THIS CONNECTION — but, per +// go-network-service-hardening.md directive 6, a remote peer's assertion must +// never be the direct trigger for terminating this process (os.Exit/log.Fatalf +// here would let a compromised or merely buggy control plane crash-loop every +// connected instance by sending a mismatched gateway_id). The connection is +// torn down and the client enters a local degraded/backoff state instead. +func (c *ControlPlaneClient) handleConnectionAck(ack *ConnectionAck) error { + if ack.GatewayID != c.gatewayID { + logger.Error("control plane asserted an unexpected gateway_id", + "expected", c.gatewayID, "asserted", ack.GatewayID) + return fmt.Errorf("gateway_id mismatch from control plane — refusing to proceed") + } + return nil +} + +// Caller treats a mismatch as a permanent failure of THIS CONNECTION, not of +// the process: it refuses to adopt the asserted identity, closes the +// connection, and marks the client degraded (surfaced via a local +// health/readiness endpoint, with backoff before reconnecting) rather than +// calling os.Exit/log.Fatalf on the control plane's say-so. +if err := client.handleConnectionAck(ack); err != nil { + logger.Error("control plane identity verification failed — entering degraded mode", "error", err) + c.health.degraded.Store(true) + c.closeConnection() + return // Caller's reconnect/backoff loop decides what happens next +} +``` + +> **Verification Checklist before outputting code:** +> * Does code accept an identity, ID, or configuration value asserted by a remote peer and use it without comparing it to a locally-held expectation? (If yes, add the comparison and fail closed on mismatch.) +> * Is a mismatch treated as a security event (logged distinctly, connection torn down and refused) rather than silently overwritten or ignored? (An identity mismatch from an authenticated peer is more suspicious than a mismatch from an unauthenticated one — the peer had to get *something* right to reach this point.) +> * Does the mismatch handler call `os.Exit`/`log.Fatalf` directly on a remote-asserted value? (If yes, replace with the degraded-state/circuit-breaker pattern from `go-network-service-hardening.md` directive 6 — a remote peer's assertion, even a mismatched one, must never be the direct trigger for terminating this process.) + +--- + +## GO-AUTH-017: Security-Relevant Route Matching Must Use Structural Route Identity, Not Heuristic String Matching + +### Severity + +High + +### Description + +Middleware or policy code that decides *whether* authentication/authorization applies to a request must key that decision off the route's actual registered binding or policy attachment — a structural match against the router/policy configuration — never a heuristic like "`method == POST` and the path contains the substring `mcp`". A heuristic predicate has false negatives for any request shape its author didn't anticipate, and those negatives silently *skip* the security check entirely rather than degrading gracefully or erroring. + +### Rationale + +A security gate keyed off a request-shape heuristic (e.g. matching on HTTP method plus a path substring) has false negatives for any request shape its author didn't anticipate — and those negatives silently skip the security check entirely, with no error or log signal that anything was bypassed. + +### Non-Compliant Code + +```go +// ERROR: whether auth applies is decided by a request-shape heuristic. Any +// request that doesn't match this exact shape skips auth/authz entirely, +// silently, with no indication that a check was bypassed. +func MCPAuthMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "mcp") { + if !isAuthenticated(r) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + } + next.ServeHTTP(w, r) // Any other method/path combination — no check at all + }) +} +``` + +### Compliant Code + +```go +// CORRECT: the decision is keyed off the route's actual registered binding — +// resolved from router/policy configuration — not a guess about request shape. +// A request that is in-scope for MCP handling but doesn't parse into the +// expected structural shape is a failure to be denied, not a check to skip. +type RouteBinding struct { + Kind string // "mcp", "rest", "llm-proxy", ... + RequiresAuth bool +} + +// ResolveRouteBinding distinguishes two failure shapes: a request that falls +// entirely outside any namespace this middleware governs (matched=false — +// let normal routing/not-found handling take it) versus a request that DOES +// target a protected namespace but couldn't be parsed into a valid structural +// binding (err != nil — deny by default, per GO-AUTH-017's own rationale). +// Collapsing both into a blanket 403 would turn every unrelated 404 into a +// Forbidden response, which is not what "deny by default" is meant to cover. +func ResolveRouteBinding(r *http.Request, router *Router) (binding RouteBinding, matched bool, err error) { + binding, ok := router.Match(r) // Structural match against registered routes + if !ok { + if router.IsProtectedNamespace(r) { + return RouteBinding{}, true, fmt.Errorf("request targets a protected namespace but did not match a registered route binding") + } + return RouteBinding{}, false, nil // Outside any namespace this middleware governs + } + return binding, true, nil +} + +func AuthMiddleware(router *Router, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + binding, matched, err := ResolveRouteBinding(r, router) + if err != nil { + // Confirmed protected namespace, unresolvable shape — deny, don't pass through. + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + if !matched { + // Not in any namespace this middleware protects — fall through to + // normal routing (e.g. a 404), not a 403. + next.ServeHTTP(w, r) + return + } + if binding.RequiresAuth && !isAuthenticated(r) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + next.ServeHTTP(w, r) + }) +} +``` + +> **Verification Checklist before outputting code:** +> * Does a security gate (auth, authz, ACL) decide whether it applies based on `r.Method`/`strings.Contains(r.URL.Path, ...)` or similar request-shape heuristics, rather than a structural match against registered routes/policy bindings? (If yes, replace with a resolution against the actual routing/policy configuration.) +> * Is the predicate used consistently across every middleware in the same protection chain (e.g. auth, authz, and ACL for the same feature)? (Divergent heuristics across sibling middlewares — even if each looks reasonable alone — can each miss a different edge case, compounding the gap.) +> * When a request is in a security-sensitive namespace but fails to match the expected structural shape, does the code deny by default rather than silently continuing? (An unrecognized shape must be treated as untrusted, not as "not applicable.") + +--- + +## GO-AUTH-018: Fail-Closed on Security-Critical File and Socket Permission Checks + +### Severity + +High + +### Description + +When code checks the permission bits of a security-critical file or Unix domain socket (a master encryption key, a private key, an IPC socket used for privileged control), a too-permissive result must cause the operation to fail — return an error, abort startup — not merely log a warning and continue. Relying on the process umask for a newly created Unix domain socket is equivalent to not checking permissions at all, since the umask is rarely tuned specifically for that socket. + +### Rationale + +A permission check that only warns still proceeds to load and use the security-critical material regardless of the result, which is equivalent to not checking at all. Relying on the ambient umask for a newly created socket has the same effect — it depends entirely on environment defaults rather than an explicit, verified check. + +### Non-Compliant Code + +```go +// ERROR: detects an overly permissive key file but only warns — the key is +// still loaded and used. +func LoadMasterKey(path string) ([]byte, error) { + info, err := os.Stat(path) + if err != nil { + return nil, err + } + if info.Mode().Perm()&0o077 != 0 { + log.Warnf("key file %s has overly permissive permissions %o", path, info.Mode().Perm()) + } + return os.ReadFile(path) // Proceeds regardless +} +``` + +```python +# ERROR: Unix domain socket created with no explicit chmod — relies on the +# ambient umask, which is typically 022 (world-readable/executable). +server.add_insecure_port(f"unix://{socket_path}") +server.start() +``` + +### Compliant Code + +```go +// CORRECT: an overly permissive key file is a hard failure, not a warning, +// with a narrowly-scoped, off-by-default development opt-out. +func LoadMasterKey(path string, devMode bool) ([]byte, error) { + info, err := os.Stat(path) + if err != nil { + return nil, err + } + if info.Mode().Perm()&0o077 != 0 { + if !devMode { + return nil, fmt.Errorf("key file %s permissions %o are too permissive — refusing to load", + path, info.Mode().Perm()) + } + log.Warnf("DEVELOPMENT MODE: loading key file %s despite permissive permissions %o", path, info.Mode().Perm()) + } + return os.ReadFile(path) +} +``` + +```python +# CORRECT: the socket is created inside a directory whose own permissions +# (0o700, owner-only) already deny access to anyone but the owning user, AND +# an umask restrictive enough for the socket file itself is applied around +# the bind call. `add_insecure_port` creates and binds the socket file +# synchronously at call time — a chmod() issued only AFTER that call still +# leaves a real (if brief) TOCTOU window where the file exists with default +# umask permissions, so the fix must make the file's permissions correct AT +# CREATION TIME, not just after it. The directory permission is the primary +# guarantee; the chmod afterward is defense-in-depth for the file itself, +# not a substitute for it. +socket_dir = os.path.dirname(socket_path) +os.makedirs(socket_dir, mode=0o700, exist_ok=True) +os.chmod(socket_dir, 0o700) # Enforce even if the directory already existed + +old_umask = os.umask(0o117) # Restrict newly created files to 0o660 from the instant of creation +try: + server.add_insecure_port(f"unix://{socket_path}") +finally: + os.umask(old_umask) # Restore — do not leave the process-wide umask narrowed + +try: + os.chmod(socket_path, 0o660) # Defense-in-depth: assert the exact mode explicitly +except OSError as e: + logger.error("Failed to set socket permissions on %s: %s", socket_path, e) + raise # Abort startup — do not serve on a socket with unverified permissions +server.start() +``` + +> **Verification Checklist before outputting code:** +> * Does a permission check on a security-critical file or socket log a warning but still proceed when the check fails? (If yes, make it a hard failure, with at most a narrowly-scoped, off-by-default dev opt-out.) +> * Is a newly created Unix domain socket's restrictive mode established AT CREATION TIME (via a narrowed `umask` around the bind call and/or a `0o700` parent directory), not merely `chmod`'d after the fact? (A `chmod` issued only after `add_insecure_port`/`bind` leaves a real TOCTOU window — the socket exists under default umask permissions from the moment it's created until the `chmod` call lands.) +> * Is the permission check re-verified periodically at runtime for long-lived processes, or only once at startup? (A file's permissions can change after a process has already loaded it — consider a periodic re-check for high-value key material.) \ No newline at end of file diff --git a/.claude/rules/error-handling.md b/.claude/rules/error-handling.md index c5d94089e..0c3a3db77 100644 --- a/.claude/rules/error-handling.md +++ b/.claude/rules/error-handling.md @@ -37,6 +37,11 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code resp * *Note:* Internal logs can log the specific reason (e.g., "token expired") for debugging, but the HTTP response writer must remain completely generic to prevent credential probing. +### 5. Secret and Sensitive-Handle Non-Disclosure + +* **Never Echo a Secret/Resource Handle Back on Resolution Failure:** An error path that fails to resolve a secret, key, credential, or similarly sensitive handle must not include that handle — or any substring of it — in the client-facing response body, even though the handle is not the secret's *value*. A handle is frequently sufficient to confirm the existence (or non-existence) of a specific tenant's resource, which is an enumeration primitive in its own right, and can be correlated with other leaked data. Do not write the raw handle into the standard internal-error log either — that log is typically readable by a broad engineering audience and often forwarded to a third-party log aggregation platform, the same concern GO-AUTH-003 raises for raw tokens. Log a redacted or keyed-hash form of the handle for correlation, and reserve the raw handle for a narrowly access-controlled audit sink, used only when forensic investigation strictly requires it. +* **Uniform Response Shape for Present-vs-Absent Resources:** Where practical, make the client-facing failure response — and its approximate latency — the same whether a referenced secret/resource exists but failed to resolve for an internal reason, or does not exist at all. This is the same unified-response principle as Directive 4 (constant-response auth failures), applied to any resource-existence-sensitive lookup, not only login. + --- ## Code Examples for Enforcement @@ -44,25 +49,34 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code resp ### ❌ Anti-Pattern (What to Reject) ```go -// BAD: Leaks internal DB state, uses vendor headers, reveals specific auth failure, and hardcodes source tags. +// BAD: Leaks internal DB state, uses a vendor header, reveals specific auth failure, and hardcodes source tags. func HandleLogin(w http.ResponseWriter, r *http.Request) { err := authenticateUser(r) if err == ErrTokenExpired { - w.Header().Set("X-AWS-Gateway-Error", "true") + w.Header().Set("X-AWS-Gateway-Error", "true") // Leaky vendor header w.WriteHeader(http.StatusUnauthorized) json.NewEncoder(w).Encode(map[string]string{ - "code": "AUTH_FAILED_EXPIRED_TOKEN_MAIN_GO_L82", - "message": "Your token has expired. Please log in again.", + "code": "AUTH_FAILED_EXPIRED_TOKEN_MAIN_GO_L82", // Source-tagged, guessable ID + "message": "Your token has expired. Please log in again.", // Reveals specific failure reason }) return } if err == sql.ErrNoRows { - w.WriteHeader(http.StatusNotFound) - json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) + json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) // Raw DB error exposed return } } +// BAD: secret-resolution failure echoes the secret handle back to the caller — +// an enumeration primitive even though the secret's value itself isn't leaked. +func HandleTemplateSecretResolution(w http.ResponseWriter, handle string, err error) { + if err != nil { + json.NewEncoder(w).Encode(map[string]string{ + "error": fmt.Sprintf("failed to resolve secret %q: %v", handle, err), + }) + } +} + ``` ### Best Practice (What to Generate) @@ -91,6 +105,34 @@ func HandleLogin(w http.ResponseWriter, r *http.Request) { } } +// GOOD: the standard internal log gets only a keyed-hash correlation ID for +// the handle, never the raw handle itself — that log is read by a broad +// engineering audience and often forwarded to a third-party aggregator (the +// same concern GO-AUTH-003 raises for tokens). The raw handle goes only to +// auditLogger, a narrowly access-controlled sink, for forensic escalation. +// The client-facing response stays sterile — identical in shape whether the +// handle doesn't exist or exists but failed to resolve for another reason. +func HandleTemplateSecretResolution(ctx context.Context, w http.ResponseWriter, handle string, err error) { + if err != nil { + logger.LogInternalError(ctx, "secret resolution failed for handle_hash %s: %v", hashHandle(handle), err) + auditLogger.Record(ctx, "secret_resolution_failed", handle, err) // Restricted audit sink only + w.WriteHeader(http.StatusUnprocessableEntity) + json.NewEncoder(w).Encode(map[string]string{ + "error": "resolution_failed", + "message": "The referenced secret could not be resolved.", // No handle, no distinction by cause + }) + return + } +} + +// hashHandle gives standard logs a stable correlation identifier without +// disclosing the handle itself to their broad readership. +func hashHandle(handle string) string { + mac := hmac.New(sha256.New, handleLogHMACKey) // Key sourced from config/secret store, never hardcoded + mac.Write([]byte(handle)) + return hex.EncodeToString(mac.Sum(nil))[:16] +} + ``` --- @@ -99,5 +141,5 @@ func HandleLogin(w http.ResponseWriter, r *http.Request) { > * Does this error message reveal *why* the auth failed to the client? (If yes, make it generic). > * Does the generated ID contain hardcoded source markers? (If yes, use a random crypto string/UUID). > * Are there any `X-Amz` or similar infrastructure headers bleeding through? (If yes, strip them). -> -> \ No newline at end of file +> * Does any client-facing error response include a secret handle, key identifier, or other sensitive resource reference — even without the underlying secret value? (If yes, strip it from the response body.) Does the standard internal-error log then get the raw handle instead? (Log a redacted/keyed-hash form there, and reserve the raw handle for a narrowly access-controlled audit sink.) +> \ No newline at end of file diff --git a/.claude/rules/file-access.md b/.claude/rules/file-access.md index 8e43dd4dc..dc7b18972 100644 --- a/.claude/rules/file-access.md +++ b/.claude/rules/file-access.md @@ -43,8 +43,15 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that * **Content-Sniff Before Trusting an Extension or Declared MIME Type:** Validate an uploaded file's actual byte content (magic-byte/structure sniffing) against an explicit allowlist of accepted types before storing or processing it — never trust the client-declared `Content-Type` header or the filename extension alone. * **Never Feed User Input to a Script/Expression/Template Engine Without a Sandbox:** If a feature accepts a script, expression, or template body from a request (a policy mediator, a transformation rule, an "execute this snippet" feature), never `eval`-equivalent it against the full language runtime. Execute it only inside an engine configured with an explicit allowlist of reachable classes/methods/built-ins — a blocklist of "dangerous" symbols is not sufficient, because the reachable surface of a general-purpose runtime is too large to enumerate exhaustively. * **Allowlist, Not Blocklist, for Reflective/Class Access:** Where a scripting or mediation feature must expose part of the Go runtime or a plugin API to user-supplied code, gate it with an explicit allowlist of permitted symbols. A blocklist approach only stops the specific bypasses already known at the time it was written. +* **Built-in Template Functions That Access Secrets or Environment Are Not Exempt:** A template-engine builtin such as `{{ secret "handle" }}` or `{{ env "NAME" }}` is itself a reflective-access primitive under this directive, even when the surrounding engine is otherwise sandboxed against arbitrary code execution. Scope `env`-style functions to an explicit allowlist of permitted variable names (reject and log any name not on the list — never return an unlisted environment variable's value). Scope `secret`-style functions to check the requesting resource's own owner/tenant handle against the secret's recorded owner *before* resolving it — never resolve any secret reachable within the same tenant purely because the caller is authenticated within that tenant; require an explicit per-resource or per-grant ACL, not tenant-wide reachability. * **Treat "Admin-Only" Script Features the Same as Any Other Untrusted Input:** A feature reachable only by an authenticated administrator is still processing untrusted input from this rule's perspective — the authorization boundary controls *who* can reach the feature, not whether the feature itself is safe to execute arbitrary code. +### 7. Streaming Decompression-Bomb Protection (Not Just ZIP Archives) + +* **Cap Decompressed Output Size for Any Streamed Content-Encoding:** Any code that decompresses a `Content-Encoding: gzip`/`br`/`deflate` body passing through the service — a proxy body-transformation step, an `ext_proc`-style body phase, a request/response rewriting policy — must bound the *decompressed* output, not just the compressed input. Wrap the decompressing reader with `io.LimitReader` (or an equivalent running byte counter) sized from configuration, and reject with a `413`-equivalent response once the ceiling is exceeded, rather than accumulating an unbounded `io.ReadAll` of the decompressed stream. +* **Guard the Ratio Before Committing to Full Decompression:** Compare the compressed size (`Content-Length` or observed byte count) against the configured decompressed ceiling using a maximum allowed ratio, and reject before decompression begins if the theoretical worst case would exceed it — the same ratio-guard principle directive 4 requires for ZIP entries, applied to any streaming decompressor a request or response body passes through. +* **Streaming (Chunked) Decompression Needs the Same Ceiling as Buffered:** A streaming decompression path (a goroutine emitting decompressed chunks as they arrive) must track *cumulative* emitted bytes across the entire stream and abort the instant the cap is hit. A cap checked only once, at the start of the stream, does not protect a streaming path — the bomb is in the bytes emitted over the stream's lifetime, not in its first chunk. + --- ## Code Examples for Enforcement @@ -55,14 +62,13 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that // BAD: Path traversal, storing full path, unbounded read, zip slip, hardcoded limit. func ServeUserFile(w http.ResponseWriter, r *http.Request) { name := r.URL.Query().Get("file") - path := "/var/app/uploads/" + name // No path cleaning — traversal possible - - data, _ := os.ReadFile(path) // Reads /etc/passwd if name = "../../etc/passwd" + path := "/var/app/uploads/" + name // No path cleaning or containment check + data, _ := os.ReadFile(path) w.Write(data) } func SaveFileMeta(db *sql.DB, uploadPath string) { - db.Exec("INSERT INTO files (path) VALUES (?)", uploadPath) // Stores full path + db.Exec("INSERT INTO files (path) VALUES (?)", uploadPath) // Stores full path, not just filename } func ProcessUpload(r *http.Request) { @@ -73,17 +79,15 @@ func ProcessUpload(r *http.Request) { func ExtractZip(src, destDir string) { zr, _ := zip.OpenReader(src) for _, f := range zr.File { - outPath := filepath.Join(destDir, f.Name) // Zip slip: f.Name may be "../../evil" - os.MkdirAll(filepath.Dir(outPath), 0755) + outPath := filepath.Join(destDir, f.Name) // No entry-path validation — zip slip possible rc, _ := f.Open() out, _ := os.Create(outPath) - io.Copy(out, rc) // No decompression ratio guard + io.Copy(out, rc) // No decompression ratio guard } } func AcceptUpload(w http.ResponseWriter, r *http.Request, upload []byte, declaredType string) { - // Trusts the client-declared Content-Type / extension with no byte-level check — - // a ".png" upload containing SVG/HTML/script content is stored and later served as-is. + // Trusts the client-declared Content-Type / extension with no byte-level check. saveUploadedFile(upload, declaredType) } @@ -276,6 +280,103 @@ func RunScriptMediator(userScript string, ctx *MessageContext) error { engine := scripting.NewSandboxedEngine(allowedScriptSymbols) // Allowlist enforced by the engine itself return engine.Eval(userScript, ctx) } + +// GOOD: streaming decompression of a proxied request/response body, bounded on +// the *decompressed* side with a running byte counter, a ratio guard, and a +// hard reject rather than an unbounded io.ReadAll of the inflated stream. +type DecompressionConfig struct { + MaxDecompressedBytes int64 + MaxRatio float64 +} + +func DecompressBoundedGzip(cfg DecompressionConfig, compressedSize int64, r io.Reader) ([]byte, error) { + if cfg.MaxDecompressedBytes <= 0 { + return nil, fmt.Errorf("invalid decompression config: MaxDecompressedBytes must be positive") + } + if cfg.MaxRatio <= 0 { + return nil, fmt.Errorf("invalid decompression config: MaxRatio must be positive") + } + if compressedSize < 0 { + return nil, fmt.Errorf("invalid compressed size: must not be negative") + } + + // Ratio guard before committing to decompression at all. Computed in + // float64 and range-checked before converting to int64 — compressedSize * + // MaxRatio can exceed math.MaxInt64, and an out-of-range float-to-int64 + // conversion is undefined by the Go spec, not merely clamped. + ceiling := cfg.MaxDecompressedBytes + if compressedSize > 0 { + ratioLimitFloat := float64(compressedSize) * cfg.MaxRatio + if ratioLimitFloat > 0 && ratioLimitFloat < float64(ceiling) { + if ratioLimitFloat > float64(math.MaxInt64) { + return nil, fmt.Errorf("ratio limit overflow for compressed size %d", compressedSize) + } + ceiling = int64(ratioLimitFloat) + } + } + if ceiling >= math.MaxInt64 { + return nil, fmt.Errorf("invalid decompression ceiling: too large") + } + + gz, err := gzip.NewReader(r) + if err != nil { + return nil, fmt.Errorf("invalid gzip stream: %w", err) + } + defer gz.Close() + + limited := io.LimitReader(gz, ceiling+1) // Bounds the DECOMPRESSED side; safe from overflow since ceiling < math.MaxInt64 is checked above + data, err := io.ReadAll(limited) + if err != nil { + return nil, err + } + if int64(len(data)) > ceiling { + return nil, fmt.Errorf("decompressed body exceeds allowed size") // 413-equivalent to the caller + } + return data, nil +} + +// GOOD: a streaming (chunked) decompression path tracks cumulative emitted +// bytes across the whole stream — a cap checked only at stream start would +// not catch a bomb that expands gradually over many chunks. It also takes +// compressedSize and applies the SAME ratio guard as DecompressBoundedGzip — +// per directive 7, a streaming path needs the identical ceiling as a buffered +// one, not merely the flat MaxDecompressedBytes cap on its own. +func StreamDecompressBounded(cfg DecompressionConfig, compressedSize int64, r io.Reader, emit func([]byte) error) error { + // Ratio guard before committing to decompression at all — identical to + // DecompressBoundedGzip's guard, just applied to the streaming path too. + ratioLimit := int64(float64(compressedSize) * cfg.MaxRatio) + ceiling := cfg.MaxDecompressedBytes + if ratioLimit > 0 && ratioLimit < ceiling { + ceiling = ratioLimit + } + + gz, err := gzip.NewReader(r) + if err != nil { + return fmt.Errorf("invalid gzip stream: %w", err) + } + defer gz.Close() + + var totalEmitted int64 + buf := make([]byte, 32*1024) + for { + n, readErr := gz.Read(buf) + if n > 0 { + totalEmitted += int64(n) + if totalEmitted > ceiling { + return fmt.Errorf("decompressed stream exceeds allowed size") // Abort mid-stream + } + if err := emit(buf[:n]); err != nil { + return err + } + } + if readErr == io.EOF { + return nil + } + if readErr != nil { + return readErr + } + } +} ``` --- @@ -285,6 +386,7 @@ func RunScriptMediator(userScript string, ctx *MessageContext) error { > * Is only the bare filename (`filepath.Base`) stored in the database or any external storage? (If the full path is stored, strip it). > * Is file processing done via `io.Reader` pipelines without intermediate `os.WriteFile` / `os.TempFile`? (If disk writes exist, remove them unless a third-party library strictly requires a path). > * Are all archive entries validated against the destination root before extraction? (If not, apply `safeJoin` on every entry). +> * Does any code path decompress a `gzip`/`br`/`deflate` body (proxy transformation, `ext_proc` body phase, response rewriting) with `io.ReadAll` on the decompressed reader and no size ceiling? (If yes, wrap it per directive 7 — bound the decompressed side, not just the compressed input, and apply the same bound to streaming/chunked decompression paths.) > * Is every inbound `io.Reader` wrapped in `io.LimitReader` with a limit sourced from configuration? (If hardcoded or absent, externalize to config with a safe default). > * Is an uploaded file's actual content sniffed (`http.DetectContentType`) against an allowlist, rather than trusting the declared `Content-Type` header or filename extension? (If trusted as-is, add content-sniffing.) > * Does any feature evaluate user-supplied script/expression/template text against the full scripting-engine runtime, or against a blocklist of "dangerous" symbols? (Both are insufficient — require an explicit allowlist of reachable classes/methods enforced by the engine itself.) diff --git a/.claude/rules/go-control-plane-xds-security.md b/.claude/rules/go-control-plane-xds-security.md new file mode 100644 index 000000000..1eda13e62 --- /dev/null +++ b/.claude/rules/go-control-plane-xds-security.md @@ -0,0 +1,216 @@ +# Rule: Go xDS / Envoy Control-Plane Security Standards + +## Context & Scope + +Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code in `gateway-controller` or `gateway-runtime` that constructs or serves an xDS gRPC server speaking to Envoy or to the policy-engine runtime (`pkg/xds`, `pkg/policyxds`), generates Envoy bootstrap/listener/HTTP-connection-manager configuration (`pkg/xds/translator.go`), or configures the Envoy admin interface (`gateway-runtime/router/config/envoy-bootstrap.yaml`). + +xDS is itself a security control plane, not just a config-distribution mechanism: a spoofed or unauthenticated xDS channel can push arbitrary listener/cluster/route configuration to every data-plane instance it serves — including routes that bypass authn/authz, or upstreams that reach internal-only services. An unauthenticated Envoy admin interface allows live traffic reconfiguration and full runtime/config disclosure — including TLS private-key material when a certificate is embedded inline (`inline_bytes`) rather than served via SDS (see directive 3); Envoy redacts SDS-sourced secret material by default, but that redaction does not cover inline key bytes outside the Secret/SDS path. Treat every directive below as being at the same severity tier as `authentication_authorization.md`, because a compromise here compromises everything that rule protects downstream. + +--- + +## Directives + +### 1. TLS + Mutual TLS Are Mandatory for Every xDS gRPC Server — Never Config-Optional in Production + +* **Default `Enabled: true` for TLS:** Every xDS/policy-xDS server (`:18000`, `:18001`-style ports) must default to TLS enabled with a required client CA (`tls.RequireAndVerifyClientCert`), not TLS-disabled-by-default with an opt-in flag. +* **Refuse to Start on an Insecure Bind:** If TLS is disabled and the listener is bound to anything other than `127.0.0.1`/a loopback address, refuse to start (fatal error), per the fail-closed startup pattern in `authentication_authorization.md` GO-AUTH-011. A plaintext xDS server is acceptable only when both the controller and the runtime it serves are strictly co-located on loopback. + +### 2. Authenticate AND Authorize Every xDS Stream — mTLS Alone Is Not Authorization + +* **A Verified Client Certificate Proves Identity, Not Entitlement:** In the go-control-plane `OnStreamOpen`/`OnStreamRequest` callbacks, extract the verified peer identity (certificate SAN, or a SPIFFE ID) and reject any stream whose identity is not on an explicit allowlist bound to the expected runtime/policy-engine node. Stream callbacks that only log the connecting peer, with no accept/reject decision, are equivalent to no authorization at all — any client that can complete the TLS handshake (including one holding a leaked or over-broadly-issued cert) receives the full snapshot. +* **The Snapshot Is Sensitive:** An xDS snapshot routinely contains API-key hashes, subscription data, and full policy chains for every tenant served by that gateway — authorize the stream as carefully as you would authorize a request to read that data directly via a REST API. + +### 3. Never Embed Private Key Material Inline in an xDS Resource Body + +* **Use SDS References, Not `inline_bytes`:** Serve the downstream listener's TLS certificate and private key via a `TlsCertificateSdsSecretConfig` reference, exactly like the upstream CA path already does — never compose the raw private key as `inline_bytes` directly inside the LDS `Listener` resource. An inline key means a single successful LDS request (from any peer that clears directives 1–2) exfiltrates the listener's private key in the resource body itself, independent of any other compromise. + +### 4. Harden the Envoy Admin Interface Independently of the xDS Channel + +* **Bind to Loopback Only:** Envoy's admin listener (`:9901`-style) must bind `127.0.0.1` in the bootstrap config, never `0.0.0.0` — it exposes TLS private keys, full runtime config, and a `/runtime_modify` endpoint capable of live traffic reconfiguration. +* **Disable Runtime Modification:** Point `flags_path` (or the equivalent runtime-layering mechanism) at a read-only mount so `/runtime_modify` cannot be used even by a caller that reaches the loopback-bound admin port through some other path (a compromised sidecar, a debug shell in the pod). +* **No Network Exposure by Construction:** Ship a default-deny NetworkPolicy template for the admin port in the Helm chart, and never provision a Kubernetes `Service`/`Ingress`/`NodePort` that exposes it. + +### 5. Resource-Limit Every xDS gRPC Server + +* **Explicit Message-Size and Stream Limits:** Set `grpc.MaxRecvMsgSize`, `grpc.MaxSendMsgSize`, and `grpc.MaxConcurrentStreams` on every xDS gRPC server construction (see `go-network-service-hardening.md` directive 2 for the general form) — an unbounded default lets a single client (malicious or merely malfunctioning) exhaust memory or consume the full stream-slot budget that other clients depend on. + +### 6. Canonicalize Request Paths Before Any Route/Policy/Authz Match Happens Downstream + +* **Set Path-Normalization Options on Every Generated HCM:** When the xDS translator generates a downstream `HttpConnectionManager`, always set `NormalizePath: true`, `MergeSlashes: true`, and an explicit `PathWithEscapedSlashesAction` (`UNESCAPE_AND_REDIRECT` or `REJECT_REQUEST`). Leaving these unset means Envoy performs route, policy, and authz matching against an un-normalized path (`//`, `/./`, `/../`, `%2F`), which can desynchronize the route Envoy actually selects from the route the operator's policy/authz configuration was written against — the same class of bypass `authentication_authorization.md` GO-AUTH-004 addresses for Go's own `net/http` routing, applied here to generated Envoy config. +* **Apply Consistently Across Every Generated HCM:** The main downstream listener, any WebSub-internal listener, and any dynamic-forward-proxy HCM must all set these fields identically — add a test asserting their presence in generated config so a new listener type added later doesn't silently omit them. + +--- + +## Code Examples for Enforcement + +### ❌ Anti-Pattern (What to Reject) + +```go +// BAD: plaintext, unauthenticated xDS gRPC server. +func NewEnvoyXDSServer() *grpc.Server { + return grpc.NewServer() // no TLS credentials +} + +// BAD: TLS defaults to disabled, with no refusal to start on a non-loopback bind. +type PolicyServerTLSConfig struct { + Enabled bool // defaults false — insecure zero value +} + +// BAD: stream callback logs the peer but never makes an accept/reject decision. +func (cb *serverCallbacks) OnStreamOpen(ctx context.Context, id int64, typ string) error { + log.Infof("xDS stream opened: id=%d type=%s", id, typ) + return nil // no identity check — always allows +} + +// BAD: private key embedded directly in the LDS resource as inline_bytes. +func createDownstreamTLSContext(cert, key []byte) *tlsv3.DownstreamTlsContext { + return &tlsv3.DownstreamTlsContext{ + CommonTlsContext: &tlsv3.CommonTlsContext{ + TlsCertificates: []*tlsv3.TlsCertificate{{ + PrivateKey: &corev3.DataSource{Specifier: &corev3.DataSource_InlineBytes{InlineBytes: key}}, // exfiltratable via LDS + }}, + }, + } +} + +// BAD: Envoy admin interface bound to all interfaces. +// envoy-bootstrap.yaml +// admin: +// address: +// socket_address: { address: 0.0.0.0, port_value: 9901 } + +// BAD: no path normalization on the generated HCM. +func createListener() *listenerv3.Listener { + hcm := &hcmv3.HttpConnectionManager{ + // NormalizePath, MergeSlashes, PathWithEscapedSlashesAction left unset + } + return buildListener(hcm) +} +``` + +### Best Practice (What to Generate) + +```go +// GOOD: xDS gRPC server requires TLS with client-cert verification by +// construction — refuses to start otherwise unless bound to loopback. The +// loopback-plaintext exception is propagated into serverCallbacks so +// OnStreamOpen authorizes streams consistently with what was actually allowed +// to start — never left to independently re-derive (or contradict) that decision. +func NewEnvoyXDSServer(cfg XDSServerConfig, cb *serverCallbacks) (*grpc.Server, error) { + plaintextLoopback := !cfg.TLS.Enabled + if plaintextLoopback && !isLoopback(cfg.BindAddr) { + return nil, fmt.Errorf("xDS server TLS is disabled and bind address %q is not loopback — refusing to start", cfg.BindAddr) + } + cb.allowPlaintextLoopback = plaintextLoopback // Only ever true for a validated loopback bind + + var opts []grpc.ServerOption + if cfg.TLS.Enabled { + pool := x509.NewCertPool() + caBytes, err := os.ReadFile(cfg.TLS.ClientCAFile) + if err != nil { + return nil, fmt.Errorf("loading xDS client CA: %w", err) + } + if ok := pool.AppendCertsFromPEM(caBytes); !ok { + return nil, fmt.Errorf("xDS client CA file %q contained no valid certificates", cfg.TLS.ClientCAFile) + } + + cert, err := tls.LoadX509KeyPair(cfg.TLS.CertFile, cfg.TLS.KeyFile) + if err != nil { + return nil, fmt.Errorf("loading xDS server cert: %w", err) + } + + creds := credentials.NewTLS(&tls.Config{ + Certificates: []tls.Certificate{cert}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: pool, + }) + opts = append(opts, grpc.Creds(creds)) + } + + opts = append(opts, + grpc.MaxRecvMsgSize(50<<20), + grpc.MaxSendMsgSize(50<<20), + grpc.MaxConcurrentStreams(1000), + ) + return grpc.NewServer(opts...), nil +} + +// GOOD: the stream callback authenticates AND authorizes — a verified +// peer identity is checked against an explicit allowlist before the stream +// is allowed to proceed; anything else is rejected outright. +var allowedNodeIdentities = map[string]bool{ + "spiffe://cluster.local/ns/gw-perf/sa/gateway-runtime": true, +} + +func (cb *serverCallbacks) OnStreamOpen(ctx context.Context, id int64, typ string) error { + p, ok := peer.FromContext(ctx) + if !ok { + return status.Error(codes.Unauthenticated, "no peer information") + } + tlsInfo, isTLS := p.AuthInfo.(credentials.TLSInfo) + if !isTLS || len(tlsInfo.State.PeerCertificates) == 0 { + // Only bypass the cert check when the server was constructed with the + // validated loopback-plaintext exception (directive 1) — never as a + // fallback for a TLS handshake that simply failed to present a cert. + if cb.allowPlaintextLoopback { + return nil + } + return status.Error(codes.Unauthenticated, "no client certificate presented") + } + identity := extractSPIFFEID(tlsInfo.State.PeerCertificates[0]) + if !allowedNodeIdentities[identity] { + logger.Warn("xDS stream rejected: unrecognized node identity", "identity", identity) + return status.Error(codes.PermissionDenied, "node identity not authorized for this snapshot") + } + return nil +} + +// GOOD: downstream listener cert/key served via SDS — never inline_bytes. +func createDownstreamTLSContext(sdsSecretName string) *tlsv3.DownstreamTlsContext { + return &tlsv3.DownstreamTlsContext{ + CommonTlsContext: &tlsv3.CommonTlsContext{ + TlsCertificateSdsSecretConfigs: []*tlsv3.SdsSecretConfig{{ + Name: sdsSecretName, + SdsConfig: sdsConfigSource(), // Delivered only over the mTLS-authenticated xDS channel from directives 1-2 + }}, + }, + } +} +``` + +```yaml +# GOOD: Envoy admin bound to loopback only; runtime_modify disabled via a +# read-only flags_path; no Service/Ingress/NodePort ever exposes this port. +admin: + address: + socket_address: { address: 127.0.0.1, port_value: 9901 } +layered_runtime: + layers: + - name: static_layer + static_layer: {} + - name: disk + disk_layer: + symlink_root: /etc/envoy/runtime # Read-only mount — no /runtime_modify effect +``` + +```go +// GOOD: path normalization set explicitly on every generated HCM. +func createListener() *listenerv3.Listener { + hcm := &hcmv3.HttpConnectionManager{ + NormalizePath: wrapperspb.Bool(true), + MergeSlashes: true, + PathWithEscapedSlashesAction: hcmv3.HttpConnectionManager_UNESCAPE_AND_REDIRECT, + } + return buildListener(hcm) +} +``` + +--- + +> **Verification Checklist before outputting code:** +> * Does an xDS/policy-xDS gRPC server default to TLS-disabled, or allow a plaintext bind to a non-loopback address without refusing to start? (If yes, flip the default and add the fail-closed startup check.) +> * Does a go-control-plane stream callback (`OnStreamOpen`/`OnStreamRequest`) only log the connecting peer without making an explicit accept/reject decision against an identity allowlist? (Logging is not authorization — add the check.) +> * Is any private key composed as `inline_bytes` inside an xDS resource (LDS `Listener`, or any other resource type)? (If yes, replace with an SDS secret reference.) +> * Does the Envoy admin interface bind `0.0.0.0`, or is `/runtime_modify` reachable without a read-only `flags_path`? (If yes, bind loopback-only and lock down the runtime layer; confirm no Service/Ingress/NodePort exposes the port.) +> * Does an xDS gRPC server construction omit `MaxRecvMsgSize`/`MaxSendMsgSize`/`MaxConcurrentStreams`? (If yes, add explicit limits per `go-network-service-hardening.md` directive 2.) +> * Does the xDS translator generate an `HttpConnectionManager` without `NormalizePath`/`MergeSlashes`/`PathWithEscapedSlashesAction` set? (If yes, set all three on every HCM the translator produces, including non-primary listeners.) diff --git a/.claude/rules/go-cors-validation.md b/.claude/rules/go-cors-validation.md new file mode 100644 index 000000000..5b5454409 --- /dev/null +++ b/.claude/rules/go-cors-validation.md @@ -0,0 +1,169 @@ +# Rule: Go CORS Origin Validation Standards + +## Context & Scope + +Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that validates an incoming `Origin` request header against an operator-configured allowlist and reflects a value into `Access-Control-Allow-Origin` — most directly the `cors` policy in `gateway-runtime` (`policies/cors/cors.go`), but equally any other Go code implementing origin-based access control for cross-origin requests. The goal is to ensure an operator's plain-looking origin allowlist behaves exactly as they intend: matching only the origins they listed, not any string that happens to contain one of those origins as a substring. + +--- + +## Directives + +### 1. Treat a Plain Origin Entry as an Exact String, Not a Partial-Match Pattern + +* **Exact-Match by Default:** An allowlist entry that looks like a literal origin (`https://app.example.com`) must be compared for exact equality — scheme and host, case-insensitive per RFC 6454 — never compiled as an unanchored regular expression and matched with `Regexp.MatchString`, which succeeds for any string that merely *contains* the literal as a substring. +* **Unanchored Regex Match Is the Vulnerability, Not a Style Choice:** `regexp.MustCompile("https://app.example.com").MatchString(origin)` matches `https://app.example.com.evil.com`, `https://evil.com/?x=https://app.example.com`, and even `https://appXexample.com` (because an unescaped `.` matches any character) — none of which the operator intended to allow when they typed a plain hostname into a config file. + +### 2. If Regex Origins Are Supported at All, Require Full-String Anchoring and a Distinct Config Field + +* **Separate the Two Input Shapes:** Give literal origins and regex patterns distinct configuration fields (e.g. `allowedOrigins` for exact strings, `allowedOriginPatterns` for regexes) rather than one field that silently accepts either shape and compiles everything as a pattern. +* **Anchor Every Compiled Pattern:** When a regex field is genuinely needed (e.g. to allow a set of preview-deployment subdomains), compile it wrapped as `^(?:pattern)$` — never trust an operator-supplied pattern to already be anchored. Reject (fail policy validation / `GetPolicy`) any pattern that isn't parseable as a full-string match, rather than silently degrading to a partial match. + +### 3. Never Combine a Broad/Unanchored Origin Match With `Access-Control-Allow-Credentials: true` + +* **Credentials Mode Is the High-Impact Case:** Setting `Access-Control-Allow-Credentials: true` alongside a matched origin means the matching origin's page can read authenticated responses (cookies, session-bound data) from this API — turning a CORS misconfiguration into a direct cross-tenant data-exposure path. Reject at config-validation time any policy that sets `allowCredentials: true` together with a wildcard origin, an unanchored pattern, or any allowlist entry that isn't a single exact-match origin. + +--- + +## Code Examples for Enforcement + +### ❌ Anti-Pattern (What to Reject) + +```go +// BAD: every allowedOrigins entry is compiled as an UNANCHORED regexp and +// matched with MatchString — a plain-looking origin behaves as a partial-match +// pattern rather than an exact string, so it can match more than the operator intended. +type CORSPolicy struct { + AllowedOrigins []string + AllowCredentials bool +} + +func (p *CORSPolicy) isOriginAllowed(origin string) bool { + for _, pattern := range p.AllowedOrigins { + re, err := regexp.Compile(pattern) // No anchoring, no escaping + if err != nil { + continue + } + if re.MatchString(origin) { // Partial match anywhere in the string + return true + } + } + return false +} + +func (p *CORSPolicy) ApplyHeaders(w http.ResponseWriter, origin string) { + if p.isOriginAllowed(origin) { + w.Header().Set("Access-Control-Allow-Origin", origin) + if p.AllowCredentials { + w.Header().Set("Access-Control-Allow-Credentials", "true") // Compounds the bypass + } + } +} +``` + +### Best Practice (What to Generate) + +```go +// GOOD: literal origins are compared for exact equality; regex patterns are a +// distinct, explicitly-named field and are always anchored before compiling; +// credentials mode is rejected outright for any non-exact-match policy. +type CORSPolicy struct { + AllowedOrigins []string // Exact-match origins, e.g. "https://app.example.com" + AllowedOriginPatterns []string // Regex patterns, explicitly named, always anchored + AllowCredentials bool +} + +func (p *CORSPolicy) Validate() error { + if p.AllowCredentials { + if len(p.AllowedOriginPatterns) > 0 { + return fmt.Errorf("allowCredentials cannot be combined with allowedOriginPatterns") + } + for _, o := range p.AllowedOrigins { + if o == "*" { + return fmt.Errorf("allowCredentials cannot be combined with a wildcard origin") + } + } + } + for _, pattern := range p.AllowedOriginPatterns { + if _, err := regexp.Compile("^(?:" + pattern + ")$"); err != nil { + return fmt.Errorf("invalid origin pattern %q: %w", pattern, err) + } + } + return nil +} + +func (p *CORSPolicy) hasWildcardOrigin() bool { + for _, o := range p.AllowedOrigins { + if o == "*" { + return true + } + } + return false +} + +func (p *CORSPolicy) isOriginAllowed(origin string) bool { + normalizedOrigin := strings.ToLower(origin) + for _, exact := range p.AllowedOrigins { + // "*" is a true wildcard — Validate() guarantees it only ever coexists + // with AllowCredentials == false, so treating it as "match any origin" + // here cannot leak credentialed responses. + if exact == "*" || strings.EqualFold(exact, normalizedOrigin) { // Wildcard, or exact match — no partial-match surface at all + return true + } + } + for _, pattern := range p.AllowedOriginPatterns { + re := regexp.MustCompile("^(?:" + pattern + ")$") // Anchored — cannot partial-match + if re.MatchString(origin) { + return true + } + } + return false +} + +func (p *CORSPolicy) ApplyHeaders(w http.ResponseWriter, origin string) { + if !p.isOriginAllowed(origin) { + return + } + if p.hasWildcardOrigin() { + // A true wildcard always reflects "*" itself, never the specific + // origin, and needs no Vary — the response is identical for every + // caller. Safe without a credentials check here because Validate() + // already rejects any policy combining a wildcard with AllowCredentials. + w.Header().Set("Access-Control-Allow-Origin", "*") + return + } + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Vary", "Origin") // Prevent shared-cache origin confusion across distinct callers + if p.AllowCredentials { + w.Header().Set("Access-Control-Allow-Credentials", "true") + } +} +``` + +```go +// cors_test.go — GOOD: regression coverage for the exact substrings that +// defeated the unanchored-regex version, so a future refactor can't reopen it. +func TestIsOriginAllowed_RejectsPartialMatchBypasses(t *testing.T) { + p := &CORSPolicy{AllowedOrigins: []string{"https://app.example.com"}} + bypasses := []string{ + "https://app.example.com.evil.com", + "https://evil.com/?x=https://app.example.com", + "https://appXexample.com", + } + for _, origin := range bypasses { + if p.isOriginAllowed(origin) { + t.Errorf("origin %q should NOT be allowed by an exact-match allowlist", origin) + } + } + if !p.isOriginAllowed("https://app.example.com") { + t.Error("the exact configured origin should be allowed") + } +} +``` + +--- + +> **Verification Checklist before outputting code:** +> * Is a plain, literal-looking origin entry compiled as a regular expression and matched with `MatchString` rather than compared via exact string equality? (If yes, switch to exact comparison for the common case.) +> * If regex-style origin matching is supported, is every pattern anchored (`^(?:pattern)$`) before compilation, and rejected at config-validation time if it isn't parseable as a full-string match? (Unanchored patterns are exactly as bypassable as no anchoring at all.) +> * Does `allowCredentials: true` coexist with a wildcard origin, a regex pattern, or anything other than a single exact-match origin? (If yes, reject the policy at validation time — this combination is the highest-impact CORS misconfiguration.) +> * Is there a regression test asserting that a configured origin does NOT match superstring/substring variations of itself (e.g. `origin.evil.com`, `evil.com/?x=origin`)? (If not, add one — this is the exact failure mode this rule exists to prevent.) diff --git a/.claude/rules/go-network-service-hardening.md b/.claude/rules/go-network-service-hardening.md new file mode 100644 index 000000000..ce4338e80 --- /dev/null +++ b/.claude/rules/go-network-service-hardening.md @@ -0,0 +1,189 @@ +# Rule: Go Network Service Hardening Standards (Timeouts, Resource Limits, Resilience) + +## Context & Scope + +Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that stands up an HTTP or gRPC server (management API, admin API, metrics endpoints), a background polling loop against an external control plane (EventHub, Platform-API), or a bounded-concurrency worker pool (Python/gRPC executors, `ext_proc` request processing). The goal is to prevent resource-exhaustion denial-of-service — from malicious/slow clients, from scale-out synchronization against a shared upstream, and from a compromised or merely malfunctioning control plane — and to ensure a single misbehaving caller or component cannot degrade the service for everyone else. + +This is the service-hardening counterpart to `authentication_authorization.md` (which governs *who* may call a service) and `ssrf-prevention.md`/`xxe-xml-processing.md` (which govern *what* a service may be tricked into doing) — this rule governs how a service survives volume, slowness, and hostile timing regardless of who the caller is. + +--- + +## Directives + +### 1. Every HTTP Server Must Set Explicit Timeouts and Size Ceilings + +* **No Bare `http.ListenAndServe`:** Construct an explicit `http.Server{}` with `ReadTimeout`, `WriteTimeout`, and `IdleTimeout` set from configuration (with safe non-zero defaults) — the zero-value server has no timeouts at all, so a slow-loris-style client can hold a connection (and a goroutine) open indefinitely. +* **Cap Header and Body Size:** Set `MaxHeaderBytes` on the server, and wrap every request body a handler reads from in `http.MaxBytesReader(w, r.Body, maxBytes)` with the ceiling sourced from configuration (see `file-access.md` directive 5 for the same principle applied to file uploads specifically). +* **Default to TLS in Non-Development Builds:** Use `ListenAndServeTLS` by default; a plaintext listener must be an explicit, narrowly-scoped development-mode opt-out, not the shipped default for a management or admin surface. + +### 2. Bound Every gRPC Server's Message Size and Concurrency + +* **Set Limits on Every `grpc.NewServer(...)` Call:** `grpc.MaxRecvMsgSize`, `grpc.MaxSendMsgSize`, and `grpc.MaxConcurrentStreams` must be set explicitly — including internal control-plane/xDS servers (see `go-control-plane-xds-security.md` for xDS-specific requirements). The library defaults either have no limit or a limit sized for general-purpose RPC, not for a specific service's threat model. + +### 3. A Bounded Queue Must Sit in Front of a Bounded Worker Pool — and Be Bounded Itself + +* **Reject, Don't Queue Unboundedly:** If work is queued ahead of a semaphore/worker-pool concurrency limit (e.g. N execution slots), the queue admitting callers into that wait must have its own explicit ceiling. Once the ceiling is hit, reject new work immediately with a typed resource-exhaustion error (e.g. gRPC `RESOURCE_EXHAUSTED`, HTTP `503`) rather than growing the queue further — an unbounded queue in front of a bounded pool is not bounded concurrency, it is delayed unbounded memory growth. +* **Cap Resource Use Per Worker, and Per Invocation When Genuinely Required:** Where workers execute arbitrary or semi-trusted logic (a Python executor, a policy-engine script), apply a memory/CPU ceiling on each worker process (e.g. `resource.setrlimit` called once in the worker's init) in addition to bounding the number of concurrent workers — concurrency limits alone don't stop one request from exhausting resources by itself. Note that `resource.setrlimit` is a per-process limit: it bounds a worker for its entire lifetime, not a single request, so a long-lived worker handling many requests sequentially is capped in aggregate, not per invocation. Where a hard per-request budget is genuinely required, isolate each invocation in its own short-lived sandboxed process or container with its own enforced resource limit, rather than relying on a shared worker's process-wide `rlimit`. + +### 4. Add Jitter to Any Fixed-Interval Poll Against a Shared Upstream + +* **Desynchronize Replica Polling:** A poll loop run identically across many replicas of the same service (an EventHub poll, a control-plane heartbeat) must desynchronize with random jitter — e.g. `time.Sleep(interval + rand.N(interval/2))` (Go's `math/rand/v2`) — computed and waited *before* each fetch, not after, and never a bare fixed-interval sleep. Without jitter, scale-out produces a synchronized thundering herd against the shared upstream every time all replicas restart or roll together, including the very first fetch after a simultaneous restart — which is exactly why the jittered wait must precede the fetch rather than follow it. Validate that `interval` is positive (and large enough that `interval/2` isn't zero) before computing jitter from it. + +### 5. Tune Chained Timeouts So the Outer Bound Is Tighter Than What It Wraps, Not Looser + +* **Outer Timeout Slightly Above the Inner, Never Many Multiples Larger:** When one component's operation is itself bounded by a downstream call that has its own shorter timeout (e.g. an `ext_proc` per-message timeout wrapping a policy-engine chain that itself times out in a few seconds), set the outer timeout modestly above the inner one — not at a generic default many multiples larger. A stuck downstream call should be caught by the inner timeout, not left to hold the outer resource (a worker thread, a connection slot, a stream) for far longer than the chain it wraps could ever legitimately take. + +### 6. Do Not Let a Remote Control Plane's Instructions Terminate Your Process + +* **Terminal Statuses From a Remote Peer Trigger Degradation, Not `os.Exit`:** Treat a "permanent failure" or similarly terminal status reported by a remote control plane as a signal to enter a degraded/backoff state — an internal circuit breaker, surfaced via a local health/readiness endpoint — never as a direct trigger for process termination. A compromised, buggy, or simply momentarily-wrong control plane must not be able to use its own protocol responses to take down every connected service instance simultaneously. Reserve `os.Exit` for conditions the process itself has verified locally (e.g. a locally-detected fatal misconfiguration at startup, per `authentication_authorization.md` GO-AUTH-011), not for a remote party's assertion about this process's own fate. + +--- + +## Code Examples for Enforcement + +### ❌ Anti-Pattern (What to Reject) + +```go +// BAD: bare ListenAndServe — no timeouts, no size caps. +func StartManagementAPI(handler http.Handler) error { + return http.ListenAndServe(":8080", handler) +} + +// BAD: gRPC server with no message-size or concurrency limits. +func NewXDSServer() *grpc.Server { + return grpc.NewServer() // unbounded defaults +} + +// BAD: unbounded queue in front of a bounded semaphore. +var executionSlots = make(chan struct{}, 100) + +func Execute(req *ExecRequest) error { + executionSlots <- struct{}{} // callers queue without limit + defer func() { <-executionSlots }() + return run(req) +} + +// BAD: fixed-interval poll — no jitter across replicas. +func PollEventHub(interval time.Duration) { + for { + fetchEvents() + time.Sleep(interval) + } +} + +// BAD: outer timeout many multiples larger than the inner chain's own timeout. +const MessageTimeout = 60 * time.Second // Policy-engine chain timeout is 5s + +// BAD: a remote control plane's response directly kills the process. +func handlePlatformAPIStatus(status string) { + if status == "permanent_failure" { + os.Exit(1) // remote-controlled process termination + } +} +``` + +### Best Practice (What to Generate) + +```go +// GOOD: explicit timeouts, header/body caps, TLS by default. +func StartManagementAPI(cfg ServerConfig, handler http.Handler) error { + wrapped := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, cfg.MaxBodyBytes) // Configurable ceiling + handler.ServeHTTP(w, r) + }) + + srv := &http.Server{ + Addr: cfg.Addr, + Handler: wrapped, + ReadTimeout: 60 * time.Second, + WriteTimeout: 60 * time.Second, + IdleTimeout: 120 * time.Second, + MaxHeaderBytes: 1 << 20, + } + + if cfg.DevelopmentMode { + log.Warn("DEVELOPMENT MODE: serving management API over plaintext HTTP") + return srv.ListenAndServe() + } + return srv.ListenAndServeTLS(cfg.CertFile, cfg.KeyFile) // TLS is the production default +} + +// GOOD: explicit gRPC resource limits. +func NewXDSServer() *grpc.Server { + return grpc.NewServer( + grpc.MaxRecvMsgSize(50<<20), + grpc.MaxSendMsgSize(50<<20), + grpc.MaxConcurrentStreams(1000), + ) +} + +// GOOD: the admission queue ahead of the concurrency semaphore is itself +// bounded — new work is rejected immediately once the ceiling is hit. +type BoundedExecutor struct { + slots chan struct{} + pending int64 + maxPending int64 +} + +func (e *BoundedExecutor) Execute(req *ExecRequest) error { + if atomic.AddInt64(&e.pending, 1) > e.maxPending { + atomic.AddInt64(&e.pending, -1) + return status.Error(codes.ResourceExhausted, "too many pending executions") + } + defer atomic.AddInt64(&e.pending, -1) + + e.slots <- struct{}{} + defer func() { <-e.slots }() + return run(req) +} + +// GOOD: jittered polling — desynchronizes replicas instead of a thundering +// herd, including on simultaneous restart. The jittered wait happens BEFORE +// each fetch, not after, so replicas that all boot at once don't immediately +// stampede the shared upstream on their very first call. +func PollEventHub(interval time.Duration) error { + if interval <= 0 { + return fmt.Errorf("poll interval must be positive, got %s", interval) + } + for { + delay := interval + if half := interval / 2; half > 0 { // Guards against a too-small interval where half rounds to zero + delay += rand.N(half) // math/rand/v2 — real, supported random-duration construction + } + time.Sleep(delay) + fetchEvents() + } +} + +// GOOD: outer timeout set modestly above the inner chain's own timeout — +// a stuck downstream call is caught by the chain's 5s timeout, not by holding +// the ext_proc worker for 12x longer than necessary. +const ChainTimeout = 5 * time.Second +const MessageTimeout = 10 * time.Second // Generous vs ChainTimeout, well below worker-pool exhaustion threshold + +// GOOD: a remote control plane's terminal status degrades this instance +// locally — it never triggers process termination on the control plane's say-so. +type ControlPlaneHealth struct { + degraded atomic.Bool +} + +func (h *ControlPlaneHealth) handlePlatformAPIStatus(status string) { + if status == "permanent_failure" { + h.degraded.Store(true) // Surfaced via /health; a circuit breaker backs off reconnect attempts + logger.Error("control plane reported a permanent failure status — entering degraded mode") + return + } + h.degraded.Store(false) +} +``` + +--- + +> **Verification Checklist before outputting code:** +> * Does an `http.Server` omit `ReadTimeout`/`WriteTimeout`/`IdleTimeout`, or use bare `http.ListenAndServe`? (If yes, construct an explicit `http.Server` with all three set from configuration.) +> * Is every request body read without first wrapping it in `http.MaxBytesReader`? (If yes, add a configurable ceiling.) +> * Does a `grpc.NewServer(...)` call omit `MaxRecvMsgSize`/`MaxSendMsgSize`/`MaxConcurrentStreams`? (If yes, add explicit limits — including on internal/control-plane gRPC servers.) +> * Is there a queue or unbounded channel send in front of a concurrency-limiting semaphore, with no ceiling on how many callers may be waiting? (If yes, add an explicit pending-count limit that rejects new work once exceeded.) +> * Does a poll loop running across multiple replicas sleep a bare fixed interval with no randomized jitter? (If yes, add jitter sized relative to the interval.) +> * Is an outer/wrapping timeout set to a generic default that is many multiples larger than the specific downstream timeout it wraps? (If yes, tune it to modestly exceed the inner timeout instead.) +> * Does a terminal/permanent status reported by a remote control plane directly call `os.Exit`/terminate the process? (If yes, replace with a local degraded-state/circuit-breaker pattern surfaced via a health endpoint — never let a remote peer's response be the direct trigger for this process's termination.) diff --git a/.claude/rules/js-authentication-authorization.md b/.claude/rules/js-authentication-authorization.md index 51087172f..8e9dc55f4 100644 --- a/.claude/rules/js-authentication-authorization.md +++ b/.claude/rules/js-authentication-authorization.md @@ -196,7 +196,7 @@ A request to `//public/%2e%2e/private/secret` can bypass a guard checking `req.p // BAD: Raw string prefix check on req.url — bypassable with encoded traversals app.use((req, res, next) => { if (req.url.startsWith('/public/')) { - return next(); // Skips auth — bypassable via /public/%2e%2e/admin + return next(); // Skips auth — bypassable via an encoded traversal sequence } authMiddleware(req, res, next); }); @@ -477,7 +477,7 @@ async function searchTenantsHandler(req, res, next) { const { name } = req.query; const [tenants] = await sequelize.query( `SELECT id, name, status FROM tenants WHERE name LIKE '%${name}%'` - ); // name = "%' OR '1'='1" defeats the filter entirely + ); // Injectable — filter can be defeated res.json(tenants); } @@ -646,8 +646,7 @@ The JavaScript counterpart of GO-AUTH-010. Open redirect via weak callback URL v ### Non-Compliant Code ```js -// BAD: Same-host substring check — bypassable via -// "https://portal.example.com.attacker.com" or "https://portal.example.com@attacker.com". +// BAD: Same-host substring check — bypassable via a crafted lookalike/userinfo host. app.get('/auth/callback', (req, res) => { const returnTo = req.query.returnTo || '/'; if (returnTo.includes('portal.example.com')) { diff --git a/.claude/rules/js-error-handling.md b/.claude/rules/js-error-handling.md index 596bfd206..fb68eff68 100644 --- a/.claude/rules/js-error-handling.md +++ b/.claude/rules/js-error-handling.md @@ -45,8 +45,8 @@ Apply this rule whenever writing, refactoring, or reviewing JavaScript (`.js`) c ### ❌ Anti-Pattern (What to Reject) ```js -// BAD: Leaks Sequelize error, sets X-Powered-By equivalent header, reveals auth failure reason, -// uses source-tagged tracking ID. +// BAD: Leaks Sequelize error, sets a leaky header, reveals auth failure reason, +// uses a source-tagged tracking ID. app.post('/login', async (req, res) => { try { const user = await User.findOne({ where: { email: req.body.email } }); @@ -55,14 +55,13 @@ app.post('/login', async (req, res) => { } const valid = await bcrypt.compare(req.body.password, user.password); if (!valid) { - return res.status(401).json({ error: 'Wrong password' }); // Enumeration leak + return res.status(401).json({ error: 'Wrong password' }); // Distinct response — same enumeration leak } - res.json({ token: generateToken(user) }); } catch (err) { res.setHeader('X-Error-Source', 'auth-service-login-handler'); // Leaky header res.status(500).json({ - error: err.message, // Exposes raw DB/stack info - code: `AUTH_ROUTE_LOGIN_L18_${Date.now()}`, // Source-tagged, guessable ID + error: err.message, // Exposes raw DB/stack info + code: `AUTH_ROUTE_LOGIN_L18_${Date.now()}`, // Source-tagged, guessable ID stack: err.stack, // Stack trace in response }); } diff --git a/.claude/rules/js-file-access.md b/.claude/rules/js-file-access.md index a8354804c..27c3dbfee 100644 --- a/.claude/rules/js-file-access.md +++ b/.claude/rules/js-file-access.md @@ -52,30 +52,29 @@ Apply this rule whenever writing, refactoring, or reviewing JavaScript (`.js`) c ### ❌ Anti-Pattern (What to Reject) ```js -// BAD: Path traversal — user controls the path completely +// BAD: Path traversal — user input concatenated directly into a path app.get('/files', (req, res) => { - const filePath = './uploads/' + req.query.name; // ../../etc/passwd bypasses root + const filePath = './uploads/' + req.query.name; // No containment check res.sendFile(path.resolve(filePath)); }); -// BAD: Storing full path in DB -await FileModel.create({ filePath: req.file.originalname }); // may contain /path/evil +// BAD: Persisting the client-controlled filename as-is (no path.basename()) instead of a sanitized bare filename +await FileModel.create({ filePath: req.file.originalname }); // BAD: Writing upload buffer to disk with a user-supplied name fs.writeFile(`/tmp/${req.file.originalname}`, req.file.buffer, callback); -// BAD: ZIP extraction with no zip-slip protection, hardcoded limit +// BAD: ZIP extraction with no zip-slip protection or entry limit const zip = await unzipper.Open.buffer(req.file.buffer); for (const entry of zip.files) { - const outPath = path.join('/var/app/content', entry.path); // Zip slip - entry.stream().pipe(fs.createWriteStream(outPath)); // Unbounded decompression + const outPath = path.join('/var/app/content', entry.path); // Unvalidated entry path + entry.stream().pipe(fs.createWriteStream(outPath)); // Unbounded decompression } -// BAD: multer with hardcoded 50 MB limit +// BAD: multer with a hardcoded size limit instead of config-sourced const upload = multer({ storage: multer.memoryStorage(), limits: { fileSize: 50 * 1024 * 1024 } }); -// BAD: Trusts the client-declared mimetype with no byte-level check, and stores/serves -// an SVG unmodified — the stored-XSS-via-SVG-upload pattern. +// BAD: Trusts the client-declared mimetype and stores/serves an upload unmodified app.post('/apis/:id/icon', upload.single('icon'), async (req, res) => { await ApiIcon.create({ apiId: req.params.id, data: req.file.buffer, mimeType: req.file.mimetype }); res.status(201).send(); @@ -84,8 +83,7 @@ app.post('/apis/:id/icon', upload.single('icon'), async (req, res) => { // BAD: Renders user-uploaded content as a template — Server-Side Template Injection. const ejs = require('ejs'); app.post('/docs/preview', upload.single('doc'), (req, res) => { - const rendered = ejs.render(req.file.buffer.toString('utf8')); // Arbitrary code execution - res.send(rendered); + res.send(ejs.render(req.file.buffer.toString('utf8'))); // Compiles untrusted input as code }); ``` diff --git a/.claude/rules/js-output-encoding-xss.md b/.claude/rules/js-output-encoding-xss.md index b85d588ff..662618c28 100644 --- a/.claude/rules/js-output-encoding-xss.md +++ b/.claude/rules/js-output-encoding-xss.md @@ -40,14 +40,11 @@ The consistent root cause is untrusted input reaching an HTML or SVG context wit ### ❌ Anti-Pattern (What to Reject) ```js -// BAD: Reflects a query parameter into an error page with no encoding — classic -// reflected XSS in an authentication/search-style endpoint. +// BAD: Reflects a query parameter into an HTML response with no encoding — +// reflected XSS via raw string interpolation. app.get('/auth/login', (req, res) => { const returnTo = req.query.returnTo || ''; - res.send(`
-You will be redirected to: ${returnTo}
- - `); // returnTo is interpolated raw into the HTML body + res.send(`You will be redirected to: ${returnTo}
`); }); // BAD: Renders user-authored Markdown/HTML for API documentation with no sanitization. @@ -56,15 +53,10 @@ app.get('/apis/:id/docs', async (req, res) => { res.render('docs', { htmlContent: doc.renderedHtml }); // Passed to <%- htmlContent %> in the view }); -// BAD: Accepts and serves an uploaded SVG as-is from the same origin as the -// authenticated session — stored XSS via SVG upload. -app.post('/apis/:id/icon', upload.single('icon'), async (req, res) => { - await ApiIcon.create({ apiId: req.params.id, data: req.file.buffer, mimeType: req.file.mimetype }); - res.status(201).send(); -}); +// BAD: Serves a stored SVG upload inline, unsanitized — stored XSS via SVG upload. app.get('/apis/:id/icon', async (req, res) => { const icon = await ApiIcon.findOne({ where: { apiId: req.params.id } }); - res.set('Content-Type', icon.mimeType).send(icon.data); // Renders inline — SVG script executes + res.set('Content-Type', icon.mimeType).send(icon.data); // Renders inline, unsanitized }); ``` diff --git a/.claude/rules/js-post-quantum-cryptography.md b/.claude/rules/js-post-quantum-cryptography.md index 863a8063a..945e07177 100644 --- a/.claude/rules/js-post-quantum-cryptography.md +++ b/.claude/rules/js-post-quantum-cryptography.md @@ -59,30 +59,27 @@ Use security level `-768` / `dilithium3` (NIST Level 3) as minimum. Escalate to ### ❌ Anti-Pattern (What to Reject) ```js -// BAD: ECDH key exchange — quantum-vulnerable +// BAD: quantum-vulnerable key exchange and signing, no PQC migration const ecdh = crypto.createECDH('prime256v1'); -ecdh.generateKeys(); -const sharedSecret = ecdh.computeSecret(peerPublicKey); // Broken by Shor's algorithm +const sharedSecret = ecdh.computeSecret(peerPublicKey); // quantum-vulnerable, no TODO(pqc) -// BAD: RSA encryption — quantum-vulnerable -const { publicKey, privateKey } = crypto.generateKeyPairSync('rsa', { modulusLength: 4096 }); -const ciphertext = crypto.publicEncrypt(publicKey, plaintext); +// privateKey here is an RSA private key — RSA-SHA256 (RSASSA-PKCS1-v1_5) is +// the quantum-vulnerable signing scheme Directive 1 prohibits for new code. +function signLegacy(data, rsaPrivateKey) { + return crypto.sign('RSA-SHA256', data, rsaPrivateKey); // undocumented legacy algorithm +} -// BAD: PQC KEM used without classical hybrid -const { cipherText, sharedSecret } = kyber768.encapsulate(recipientPub); -// No X25519 leg — no fallback if ML-KEM is broken +// BAD: PQC used without classical hybrid, and nonce reuse +const { sharedSecret: pqcOnly } = kyber768.encapsulate(recipientPub); // no X25519 hybrid leg -// BAD: AES-128 for long-lived keys -const key = crypto.randomBytes(16); // AES-128 — only ~80-bit post-quantum security +const key = crypto.randomBytes(32); // AES-256 key size, matches 'aes-256-gcm' +const staticNonce = crypto.randomBytes(12); // Generated once, then reused below — breaks GCM -// BAD: Nonce reuse -const FIXED_NONCE = Buffer.alloc(12, 0); // Never reuse — nonce collision breaks AES-GCM -const cipher = crypto.createCipheriv('aes-256-gcm', key, FIXED_NONCE); +const cipher1 = crypto.createCipheriv('aes-256-gcm', key, staticNonce); +const ciphertext1 = Buffer.concat([cipher1.update(Buffer.from('message one')), cipher1.final()]); -// BAD: Undocumented legacy algorithm -function signLegacy(data, privateKey) { - return crypto.sign('SHA256', data, privateKey); // No TODO(pqc) — silently quantum-vulnerable -} +const cipher2 = crypto.createCipheriv('aes-256-gcm', key, staticNonce); // Same key + nonce reused — GCM keystream repeats +const ciphertext2 = Buffer.concat([cipher2.update(Buffer.from('message two')), cipher2.final()]); ``` ### Best Practice (What to Generate) diff --git a/.claude/rules/js-ssrf-prevention.md b/.claude/rules/js-ssrf-prevention.md index 289dd79ff..5e7d468d0 100644 --- a/.claude/rules/js-ssrf-prevention.md +++ b/.claude/rules/js-ssrf-prevention.md @@ -48,9 +48,8 @@ app.post('/api/specs/import-from-url', async (req, res) => { res.json(response.data); }); -// BAD: String-only hostname check — bypassable via DNS rebinding or IP-literal encoding -// (e.g. "http://0177.0.0.1/", decimal "http://2130706433/", or a domain that later -// re-resolves to 169.254.169.254 after this check has already passed). +// BAD: String-only hostname check — bypassable via DNS rebinding or an +// alternate IP-literal encoding of a denied address. function isSafeUrl(raw) { const { hostname } = new URL(raw); return hostname !== 'localhost' && !hostname.startsWith('127.'); diff --git a/.claude/rules/js-xxe-xml-processing.md b/.claude/rules/js-xxe-xml-processing.md index dff476250..4e6220f90 100644 --- a/.claude/rules/js-xxe-xml-processing.md +++ b/.claude/rules/js-xxe-xml-processing.md @@ -48,11 +48,11 @@ app.post('/api/specs/wsdl-import', upload.single('wsdl'), async (req, res) => { res.json(result); // No DOCTYPE check, no depth limit, no parse timeout }); -// BAD: schema location taken from the document itself and fetched — XXE and SSRF -// in the same code path. +// BAD: schema location taken from the untrusted document itself and fetched — +// XXE and SSRF in the same code path. async function validateAgainstDeclaredSchema(xmlString) { const schemaLocation = extractSchemaLocation(xmlString); - const schemaResp = await axios.get(schemaLocation); // Fetches an attacker-chosen URI + const schemaResp = await axios.get(schemaLocation); return validateAgainstSchema(xmlString, schemaResp.data); } ``` diff --git a/.claude/rules/post-quantum-cryptography.md b/.claude/rules/post-quantum-cryptography.md index db42325b4..891bbe788 100644 --- a/.claude/rules/post-quantum-cryptography.md +++ b/.claude/rules/post-quantum-cryptography.md @@ -56,30 +56,20 @@ Use security level `-768` / `mode3` (NIST Level 3) as the minimum. Escalate to ` ### ❌ Anti-Pattern (What to Reject) ```go -// BAD: ECDH key exchange — quantum-vulnerable +// BAD: quantum-vulnerable key exchange and signing, no PQC migration priv, _ := ecdh.P256().GenerateKey(rand.Reader) -pub := priv.PublicKey() -shared, _ := priv.ECDH(peerPub) // Broken by Shor's algorithm +shared, _ := priv.ECDH(peerPub) // quantum-vulnerable, no TODO(pqc) -// BAD: RSA encryption — quantum-vulnerable -rsaKey, _ := rsa.GenerateKey(rand.Reader, 4096) -cipher, _ := rsa.EncryptOAEP(sha256.New(), rand.Reader, &rsaKey.PublicKey, plaintext, nil) - -// BAD: No hybrid — PQC used in isolation before library maturity confirmed -import "github.com/cloudflare/circl/kem/kyber/kyber768" -pub, priv, _ := kyber768.GenerateKeyPair() -ct, sharedSecret, _ := kyber768.Encapsulate(pub) // Sole key exchange — no classical fallback - -// BAD: AES-128 for long-lived session keys -key := make([]byte, 16) // AES-128: only ~80-bit post-quantum security -rand.Read(key) - -// BAD: Undocumented legacy algorithm func signLegacy(data []byte, key *rsa.PrivateKey) []byte { sig, _ := rsa.SignPKCS1v15(rand.Reader, key, crypto.SHA256, data) - return sig - // No TODO(pqc) comment — silently quantum-vulnerable + return sig // undocumented legacy algorithm } + +// BAD: PQC used without classical hybrid, and weak symmetric key size +pub, _, _ := kyber768.GenerateKeyPair(rand.Reader) +ct, sharedSecret, _ := kyber768.Scheme().Encapsulate(pub) // no X25519 hybrid leg + +key := make([]byte, 16) // AES-128 — reduced post-quantum security ``` ### Best Practice (What to Generate) diff --git a/.claude/rules/ssrf-prevention.md b/.claude/rules/ssrf-prevention.md index 4fa949fca..cda5e560c 100644 --- a/.claude/rules/ssrf-prevention.md +++ b/.claude/rules/ssrf-prevention.md @@ -2,7 +2,7 @@ ## Context & Scope -Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that causes the server to make an outbound network request whose destination, in whole or in part, is derived from user or tenant input. This includes WebSub/webhook callback delivery (`event-gateway/gateway-runtime/internal/subscription`, `event-gateway/gateway-runtime/internal/connectors/receiver/websub`), backend/upstream target resolution in the gateway, "try it" / test-invoke style features, URL-based import of specs (OpenAPI/WSDL), and any header-driven request redirection (e.g. WS-Addressing `ReplyTo`/`FaultTo`-style headers). The goal is to prevent an attacker from using the server as a proxy to reach internal-only network resources, cloud metadata endpoints, or other services the attacker could not otherwise reach directly. +Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that causes the server to make an outbound network request whose destination, in whole or in part, is derived from user or tenant input. This includes WebSub/webhook callback delivery (`event-gateway/gateway-runtime/internal/subscription`, `event-gateway/gateway-runtime/internal/connectors/receiver/websub`), backend/upstream target resolution in the gateway — including the **RestApi, Mcp, and LlmProvider/LlmProxy** upstream validators (`gateway-controller/pkg/config/api_validator.go`, `mcp_validator.go`, `llm_validator.go`) — "try it" / test-invoke style features, URL-based import of specs (OpenAPI/WSDL), any header-driven request redirection (e.g. WS-Addressing `ReplyTo`/`FaultTo`-style headers), and **URLs extracted from proxied or LLM-generated/LLM-request content** (e.g. a `url-guardrail`-style policy that dereferences links found inside model input/output). The goal is to prevent an attacker from using the server as a proxy to reach internal-only network resources, cloud metadata endpoints, or other services the attacker could not otherwise reach directly. --- @@ -36,6 +36,12 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that * **Externalize the Allowlist/Denylist:** Source the private-range denylist defaults from code (safe built-in defaults per directive 2), but allow operators to extend it via configuration for environment-specific ranges (e.g. an internal VPC CIDR). Never allow per-tenant configuration to *widen* the denylist exceptions without an explicit administrative opt-in flag that is off by default. * **Log and Reject, Don't Silently Drop:** On rejection, return a generic `400 Bad Request` / `422 Unprocessable Entity` to the caller (never reveal *why* — i.e. don't echo back "resolved to private IP" which helps an attacker map internal topology) while logging the actual resolved IP internally for audit. +### 6. One Shared Validation Helper Across Every Upstream/Backend Validator + +* **No Duplicate, Drifting Implementations:** When more than one code path validates an upstream/backend URL — a REST API backend, an MCP upstream, an LLM provider/proxy upstream, a WebSub callback — implement the private-IP/metadata/scheme checks exactly **once** in a shared helper (e.g. a `netguard`-style package) and call it from every validator. Independent, per-feature reimplementations reliably drift: the first path built gets the full private-IP/metadata denylist, and a similar validator added later for a newer feature (MCP, LLM proxy) performs only a syntactic `url.Parse` check because the shared logic wasn't extracted or reused. +* **Audit Every Validator When Adding a New Upstream Kind:** Before shipping a new kind of user/tenant-configurable upstream (a new connector type, a new proxy mode), grep for every existing `validateUpstream*`/`*_validator.go` function and confirm the new one calls the same shared helper — do not write a new bespoke check "because this one is simpler." +* **Test Every Validator, Not Just the First:** Add a validator unit test per upstream kind (REST, MCP, LLM, WebSub) asserting a rejection (400/422) for loopback, RFC 1918, link-local, and cloud-metadata targets. The presence of a passing test for one validator is not evidence that sibling validators enforce the same policy. + --- ## Code Examples for Enforcement @@ -60,7 +66,7 @@ func ValidateCallbackURL(raw string) error { if err != nil { return err } - if strings.Contains(u.Host, "localhost") || strings.HasPrefix(u.Host, "127.") { + if strings.Contains(u.Host, "localhost") { // String-only check, no IP-level validation return fmt.Errorf("invalid host") } return nil // No IP-level check, no re-check at dial time @@ -207,3 +213,5 @@ func DeliverWebhook(ctx context.Context, client *http.Client, sub *Subscription, > * Does any code path honor a header- or body-supplied "reply to" / "callback" / "redirect to" address to make a second outbound request without validation? (If yes, validate it identically to directive 1–2, or reject the field outright.) > * Are webhook/callback deliveries bounded by a timeout and a response-size limit? (If not, add both — an unbounded read/hang is a resource-exhaustion vector on top of SSRF.) > * Does an error response to the caller reveal the resolved internal IP or *why* a destination was rejected? (If yes, generalize the client-facing message and log the specific reason internally only.) +> * Is there more than one upstream/backend validator (REST, MCP, LLM, WebSub) in this codebase, and do they all call the same shared private-IP/metadata-denylist helper? (If a new validator performs only a syntactic `url.Parse` check while an existing sibling validator does full IP-level validation, that is the exact drift this rule exists to prevent — see directive 6.) +> * Does a feature dereference URLs extracted from proxied or model-generated content (not just from a request field/header)? (Apply the same dial-time validation — content-derived URLs are exactly as untrusted as a request parameter.)