Feat/analytics policy inspector#96
Merged
Merged
Conversation
…, path traversal block Six security fixes addressing HIGH and CRITICAL findings from the control-plane security audit: C1+C2 — Auth + RBAC enabled by default in Helm chart: The Helm deployment.yaml now sets CP_AUTH_ENABLED=true and CP_RBAC_ENABLED=true by default. The Go code defaults remain false for local-dev, but the production chart (the deployment path) is secure-by-default. Operators must supply a bcrypt password hash via consoleAuth.passwordHash or consoleAuth.passwordHashFile. H1 — PromQL/LogQL injection via PUT /alerts/rules: The handlePutRules endpoint now rejects overrides that include a Query or Source field. Only Threshold, For, Window, Enabled, and Severity may be overridden via the API. Operators who need to change a rule's query must edit the ConfigMap directly. Custom rule names (not in the built-in catalog) are also rejected via the API — only overrides of existing defaults are allowed. H2 — Path traversal via kubeconfigPath in cluster registration: handleAddCluster now rejects kubeconfigPath from the HTTP API. The clientcmd loader reads arbitrary files from the CP pod's filesystem; operators must supply apiServer + token + caBundle instead, which are passed to the REST config without filesystem access. H3 — SSRF via handleRegisterInstance AdminURL: New validateAdminURL function (ssrf.go) checks user-supplied AdminURLs against loopback (127/8, ::1), link-local (169.254/16, fe80::/10), private RFC1918 ranges, cloud metadata endpoints (AWS/GCP/Azure/Alibaba/ECS/OCI), and unspecified addresses. DNS is resolved and every IP is validated to guard against DNS rebinding. Seven downstream consumers (proxy, aggregator, health checker, decision collector, route probes, quick test, reconciler) trust the registry's AdminURL unconditionally — the intake is the single choke point. H4 — SSRF via handleProbeURL redirect bypass + weak blocklist: The CheckRedirect callback now re-runs isBlockedHost on every redirect target, closing the redirect-bypass where an attacker redirects from a safe host to a metadata endpoint. The blocklist is expanded from 3 entries to 10, adding Azure IMDS, Alibaba IMDS, ECS task metadata, localhost, ::1, AWS IMDS IPv6, and IP-literal checks against loopback/link-local ranges via net.ParseIP. M1 — WebSocket Origin verification fail-closed warning: When CP_AUTH_ENABLED=true and CP_CONSOLE_ORIGIN is empty, the startup logs an error-level warning that WS connections will accept any origin, directing the operator to set the env var. go test ./... clean; go vet ./... clean.
…acheSize rotatableIdentifier.Identify had its own copy of the RFC 9449 / RFC 7800 cnf.jkt confirmation-claim binding check, missing the else-branch that rejects a DPoP-bound bearer token presented without a cnf.jkt claim. The two Identify paths (base and pinned-key) now share one enforceCnfBinding helper so they can't drift apart again. Also removes the replayCacheSize config field, which was parsed but never actually read: the replay cache is sized via the top-level caches: pool config, not a per-identifier field. Docs updated to point at the caches: pool instead.
… bypass The signature-verification path used byte-level scanning to locate SignedInfo/Signature/references, which encoded ECDSA signatures the wrong way (ASN.1 DER instead of the raw r||s concatenation XML-DSIG requires) and skipped C14N canonicalization entirely -- ECDSA-signed assertions never actually verified, and RSA verification wasn't spec-compliant either. Replaces it with github.com/russellhaering/goxmldsig (+ beevik/etree), which performs real canonicalization and Reference/digest handling. verifyAndExtractAssertion() validates the assertion (or falls back to a response-level signature), then re-parses business-logic fields exclusively from the goxmldsig-returned/validated element -- never a separately-parsed copy of the raw response -- so a decoy assertion can't be trusted just because *some* signature elsewhere in the document is valid. singleAssertionChild() additionally rejects any document with zero or multiple direct-child Assertions, closing decoy-as-sibling smuggling at the parsing layer. Also fixes a bypass where an empty SubjectConfirmationData@Recipient attribute skipped the Recipient check instead of failing it like any other mismatch (SAML 2.0 Profiles S4.1.4.2 requires Recipient on bearer confirmations). saml_test.go rewritten to sign fixtures with real goxmldsig-based signing instead of hand-computed digests, so tests exercise genuine XML-DSIG verification rather than a matching hand-rolled implementation.
Lookup always recomputed argon2id with the package's own hardcoded time/memory/threads constants, ignoring the m=/t=/p= cost parameters actually embedded in each stored $argon2id$ hash string -- a hash generated by a standard argon2 tool with different (but still safe) parameters never matched. parseEncoded now parses and honors those parameters, with a security floor: params below the package's own minimums are rejected at load time rather than silently downgrading verification cost. Also wires the previously-dead buildRotatableStore into the live factory via a new secrets: config array (mutually exclusive with static/hashed), using the shared keyrotation.ParseSecretsConfig format so apikey credentials can be rotated with overlapping notBefore/notAfter windows instead of requiring a hard cutover.
buildRotatableIdentifier populated a flat, state-unaware map from all configured key entries regardless of rotation lifecycle, and identifier.Identify read straight from that map -- a retired key kept verifying signatures indefinitely instead of being rejected once past its notAfter+grace window. Key resolution is now routed through an indirection (lookupKey) that the rotatable builder points at the KeySet's state-aware Get, so retired keys are actually rejected. Also adds a secrets: config array (mutually exclusive with the existing keys: map) using the shared keyrotation format, so HMAC keys can be rotated with an overlap window instead of a hard cutover.
…ecrets rotation The positive-cache TTL clamp only applied when the token's exp was in the future (d > 0); an IdP response with active: true but an exp already in the past skipped the clamp and got cached as valid for the full maxCacheTtl. Now treated as inactive (negative-cached) instead. Also wires the client secret used for introspection Basic Auth through a new resolveSecret indirection, defaulting to the static cfg.ClientSecret but backed by a KeySet when a secrets: config array is present -- clientId stays static, only the secret behind it rotates, resolved fresh on every introspection call so a retired secret stops being sent immediately.
… rotation Three fixes: - buildCookieStoreNamed applied the operator's configured cookie.sameSite to both the session cookie and the short-lived flow cookie (state + PKCE verifier). SameSite=Strict on the flow cookie breaks every login, since the IdP's redirect back to /oauth2/callback is a cross-site top-level navigation that browsers never attach a Strict cookie to. The flow cookie now forces at least Lax regardless of the configured session-cookie setting. - The JWKS background poller was bound to context.Background(), so every config hot-reload leaked the previous poller. factory is now deps-aware (RegisterIdentifierWithDeps) and binds the poller to deps.Ctx, following the same pattern already used in pkg/identity/jwt. - Wires a secrets: config array (mutually exclusive with clientSecret) through a new resolveClientSecret indirection. currentOAuthConfig() builds a per-call *oauth2.Config copy with the freshly-resolved secret for Exchange/TokenSource, and device.go's manual token-poll form-encoding uses the same resolver -- the shared i.oauth config is never mutated in place, avoiding a race under concurrent requests.
CABundleWatcher (fsnotify-based CertPool reload) existed but was never constructed by the factory -- trustedCAFiles was always loaded once, statically, at startup. Adds a watchCAFile: true config option that, when set (with exactly one trustedCAFiles entry and no trustedCAs), builds a watcher bound to the engine-lifecycle context instead of the one-shot loadCAPool(). The identifier resolves its trust root via a caPool indirection on every request rather than a pool captured once at construction, so a CA file change on disk actually takes effect. factory is now deps-aware (RegisterIdentifierWithDeps) to get access to deps.Ctx, following the same ctx-binding pattern as pkg/identity/jwt's JWKS poller. watchLoop gained a ctx.Done() case so the watcher goroutine is torn down when the engine is swapped on hot-reload instead of leaking.
isBlockedHost only matched IP literals against loopback/link-local ranges and a static 9-entry hostname list; a hostname that wasn't itself an IP literal or on that list passed straight through regardless of what IP it actually resolved to, since the function never called net.LookupIP. An attacker-registered domain resolving to a cloud metadata IP or an internal service would bypass both the initial check and the CheckRedirect revalidation. Both call sites now use validateHost (ssrf.go), which resolves the hostname and validates every returned IP against loopback/link-local/ metadata ranges. Passes allowPrivate=true to preserve this endpoint's documented, intentional support for in-cluster IdP hostnames (which commonly resolve to RFC1918 addresses) -- unlike handleRegisterInstance, which uses allowPrivate=false.
Strix Security Review1 open security finding on this PR:
Review summaryThe PR adds analytics/control-plane query paths and multiple identity rotation hardening changes. One confirmed issue remains in the new SSRF protection: SSRF guard allows unresolved hosts to bypass destination validation, which weakens the new URL probe safety checks. Updated for Reviewed by Strix |
vet Summary ReportThis report is generated by vet Policy Checks
Malicious Package AnalysisActive malicious package analysis was disabled. Learn more about enabling active package analysis Malicious Package Analysis Report
Changed PackagesChanged Packages
Policy ViolationsPackages Violating Policy[Go]
|
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
validateHost returned nil (allow) when net.LookupIP failed for a non-literal hostname, on the theory that the outbound HTTP client would fail on the same unresolvable host anyway. That's not true for an attacker who controls DNS for the queried domain: they can make the validation-time lookup fail on demand (e.g. SERVFAIL) while a later lookup from the actual outbound client succeeds with an internal or metadata IP, skipping the IP-range checks entirely. A resolution failure is now rejected instead of passed through. CWE-918. Found in code review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.