diff --git a/.golangci.yml b/.golangci.yml index 1709098..15b6399 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,40 +1,26 @@ -# golangci-lint configuration +# golangci-lint v2 configuration # https://golangci-lint.run/usage/configuration/ - +version: "2" run: - timeout: 5m issues-exit-code: 1 tests: true - output: formats: - - format: colored-line-number - print-issued-lines: true - print-linter-name: true - + text: + path: stdout + print-linter-name: true + print-issued-lines: true linters: enable: - # Default linters - - errcheck - - gosimple - - govet - - ineffassign - - staticcheck - - unused - # Additional linters - bodyclose - dogsled - dupl - errorlint - exhaustive - - exportloopref - gochecknoinits - - goconst - gocritic - gocyclo - godot - - gofmt - - goimports - goprintffuncname - gosec - misspell @@ -47,92 +33,113 @@ linters: - revive - rowserrcheck - sqlclosecheck - - stylecheck + - staticcheck - tparallel - unconvert - unparam - whitespace - -linters-settings: - errcheck: - check-type-assertions: true - check-blank: true - - govet: - enable-all: true - disable: - - fieldalignment - - gocyclo: - min-complexity: 15 - - goconst: - min-len: 3 - min-occurrences: 3 - - misspell: - locale: US - - revive: + settings: + dupl: + threshold: 150 + exhaustive: + # A switch with a default branch has made its catch-all explicit; + # don't also require every enum member to be spelled out. + default-signifies-exhaustive: true + gocritic: + disabled-checks: + - dupImport + - ifElseChain + - octalLiteral + - whyNoLint + - wrapperFunc + # controller-runtime passes spec structs by value all over; flagging + # every 160-byte struct copy is noise for an operator codebase. + - hugeParam + enabled-tags: + - diagnostic + - experimental + - opinionated + - performance + - style + gocyclo: + min-complexity: 20 + gosec: + excludes: + - G104 + - G304 + govet: + disable: + - fieldalignment + # `if err := f(); err != nil` inside a scope that already has an err + # is idiomatic Go; shadow flags all of them. + - shadow + enable-all: true + misspell: + locale: US + revive: + rules: + - name: blank-imports + - name: context-as-argument + - name: context-keys-type + - name: dot-imports + - name: error-return + - name: error-strings + - name: error-naming + - name: exported + - name: if-return + - name: increment-decrement + - name: var-naming + - name: var-declaration + - name: package-comments + - name: range + - name: receiver-naming + - name: time-naming + - name: unexported-return + - name: indent-error-flow + - name: errorf + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling rules: - - name: blank-imports - - name: context-as-argument - - name: context-keys-type - - name: dot-imports - - name: error-return - - name: error-strings - - name: error-naming - - name: exported - - name: if-return - - name: increment-decrement - - name: var-naming - - name: var-declaration - - name: package-comments - - name: range - - name: receiver-naming - - name: time-naming - - name: unexported-return - - name: indent-error-flow - - name: errorf - - gosec: - excludes: - - G104 # Audit errors not checked - - G304 # File path provided as taint input - - dupl: - threshold: 150 - - gocritic: - enabled-tags: - - diagnostic - - experimental - - opinionated - - performance - - style - disabled-checks: - - dupImport - - ifElseChain - - octalLiteral - - whyNoLint - - wrapperFunc - + - linters: + - dogsled + - dupl + - errcheck + - gocyclo + - gosec + path: _test\.go + # kubebuilder scaffolds init() for scheme registration in api packages + # and manager entrypoints — that's the supported pattern, not a smell. + - linters: + - gochecknoinits + path: ^(api|cmd)/ + # Reconcile loops are long state machines by design; the complexity + # budget elsewhere stays at the default. + - linters: + - gocyclo + path: ^internal/controller/ + - linters: + - all + path: (.*)\.gen\.go + paths: + - third_party$ + - builtin$ + - examples$ issues: - exclude-rules: - # Exclude some linters from running on tests files - - path: _test\.go - linters: - - gocyclo - - errcheck - - dupl - - gosec - - goconst - - # Exclude known issues in generated files - - path: "(.*)\\.gen\\.go" - linters: - - all - max-issues-per-linter: 0 max-same-issues: 0 new: false +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 2f1c0cf..1051c57 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -13,6 +13,7 @@ var ( GroupVersion = schema.GroupVersion{Group: "dploy.dev", Version: "v1alpha1"} // SchemeBuilder is used to add go types to the GroupVersionKind scheme. + //nolint:staticcheck // scheme.Builder is the kubebuilder-scaffolded pattern; SA1019 is aimed at hand-written api packages. SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion} // AddToScheme adds the types in this group-version to the given scheme. diff --git a/charts/dploy/values.yaml b/charts/dploy/values.yaml index 36a1b4e..e0de1fb 100644 --- a/charts/dploy/values.yaml +++ b/charts/dploy/values.yaml @@ -69,8 +69,19 @@ auth: jwtUsernameClaim: preferred_username oidcClientID: dploy oidcClientSecret: "" + + # oidcIssuer is the URL the API uses for discovery, token exchange and + # JWKS — typically the in-cluster Service URL in a Kubernetes deployment. oidcIssuer: "" + + # oidcPublicIssuer is OPTIONAL — only set it for split-horizon deployments + # where the IdP is reached via two distinct URLs: an in-cluster one for + # backend traffic (oidcIssuer above) and a public one served via the + # ingress/gateway for browser redirects. When equal to oidcIssuer (or + # empty), the API uses a single URL for everything and the split-horizon + # code path is a no-op. oidcPublicIssuer: "" + oidcRedirectURL: "" service: diff --git a/cmd/api/main.go b/cmd/api/main.go index 78f8228..176a464 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -148,7 +148,7 @@ func main() { // Admin endpoints — gated by MANAGER_ENABLED + the admin claim/value pair. // 404 when disabled, 403 to non-admin requesters. Shared manager-gate - // middleware so both routes get the same 404 behaviour off-feature. + // middleware so both routes get the same 404 behavior off-feature. managerGate := func(c *fiber.Ctx) error { if !cfg.ManagerEnabled { return c.Status(fiber.StatusNotFound).JSON(models.ErrorResponse{Error: "manager disabled"}) diff --git a/go.mod b/go.mod index b9f2c56..e88296d 100644 --- a/go.mod +++ b/go.mod @@ -52,7 +52,6 @@ require ( github.com/go-openapi/swag/typeutils v0.25.4 // indirect github.com/go-openapi/swag/yamlutils v0.25.4 // indirect github.com/google/gnostic-models v0.7.0 // indirect - github.com/gorilla/securecookie v1.1.2 // indirect github.com/huandu/xstrings v1.5.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/klauspost/compress v1.18.0 // indirect diff --git a/go.sum b/go.sum index dd6462e..f984147 100644 --- a/go.sum +++ b/go.sum @@ -98,8 +98,6 @@ github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 h1:BHT72Gu3keYf3ZEu2J github.com/google/pprof v0.0.0-20250403155104-27863c87afa6/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA= -github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo= github.com/huandu/xstrings v1.5.0 h1:2ag3IFq9ZDANvthTwTiqSSZLjDc+BedvHPAp5tJy2TI= github.com/huandu/xstrings v1.5.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= diff --git a/internal/auth/jwt.go b/internal/auth/jwt.go index f9a62db..18b2e96 100644 --- a/internal/auth/jwt.go +++ b/internal/auth/jwt.go @@ -40,16 +40,16 @@ func NewJWTValidator(jwksURL, issuer, audience, usernameClaim string) *JWTValida return &JWTValidator{verifier: verifier, usernameClaim: usernameClaim} } -// Validate verifies the token and returns (sanitizedUsername, raw claims, err). -// All cryptographic and standard-claim checks live inside Verify; the only -// dploy-specific work is pulling the configured username claim and sanitizing -// it for use as a Kubernetes label. -func (v *JWTValidator) Validate(tokenString string) (string, map[string]any, error) { +// Validate verifies the token and returns the sanitized username plus the raw +// claims. All cryptographic and standard-claim checks live inside Verify; the +// only dploy-specific work is pulling the configured username claim and +// sanitizing it for use as a Kubernetes label. +func (v *JWTValidator) Validate(tokenString string) (username string, claims map[string]any, err error) { idToken, err := v.verifier.Verify(context.Background(), tokenString) if err != nil { return "", nil, fmt.Errorf("token parsing failed: %w", err) } - claims := map[string]any{} + claims = map[string]any{} if err := idToken.Claims(&claims); err != nil { return "", nil, fmt.Errorf("decode claims: %w", err) } diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 9efdae0..cc8137e 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -1,7 +1,21 @@ +// Package auth handles JWT verification and the OIDC login flow for the dploy +// API. +// +// Both JWT verification (jwt.go) and the OIDC login flow (this file) follow +// the canonical Go OIDC pattern used by Kubernetes, Argo CD and the official +// coreos/go-oidc README example: go-oidc for discovery + ID token +// verification, golang.org/x/oauth2 for the Authorization Code + PKCE flow, +// and four short-lived HttpOnly cookies to carry state/verifier/nonce/returnUrl +// across the browser bounce. No framework, no signed-cookie key management, +// nothing dploy-specific beyond the optional split-horizon issuer support +// and the SPA's "#token=..." hand-off. package auth import ( "context" + "crypto/rand" + "crypto/subtle" + "encoding/base64" "fmt" "net/url" "strings" @@ -11,96 +25,95 @@ import ( "github.com/AYDEV-FR/dploy/internal/logger" "github.com/coreos/go-oidc/v3/oidc" "github.com/gofiber/fiber/v2" - "github.com/gorilla/securecookie" "golang.org/x/oauth2" ) -// stateBlob is what we encode + sign into the OAuth2 `state` parameter. -// Carrying the data inside the signed token keeps things stateless — no -// server-side map, no cleanup goroutine — so the only failure mode left -// is "browser took longer than stateTTL to come back", which is intended. -type stateBlob struct { - ReturnURL string - Expiry int64 // unix seconds -} - -// OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the -// configured IdP. Heavy lifting is delegated to golang.org/x/oauth2 (Code -// exchange, AuthCodeURL) and github.com/coreos/go-oidc (discovery). The only -// dploy-specific bits are split-horizon endpoint handling, a one-shot state -// map for CSRF protection, and the post-callback hash-fragment redirect. +// OIDCHandler wires the canonical go-oidc + oauth2 pair into Fiber handlers. type OIDCHandler struct { - config *config.Config oauth2Config *oauth2.Config - sc *securecookie.SecureCookie // signs+encodes the OAuth2 state blob + verifier *oidc.IDTokenVerifier + secureCookie bool } const ( - stateTTL = 10 * time.Minute discoveryTimeout = 10 * time.Second discoveryAttempts = 5 + + // flowCookieMaxAge bounds how long a user has to complete the IdP + // bounce. 10 min covers slow MFA prompts without leaving stale state + // cookies indefinitely. + flowCookieMaxAge = 10 * 60 + + cookieState = "dploy_oidc_state" + cookieVerifier = "dploy_oidc_verifier" + cookieNonce = "dploy_oidc_nonce" + cookieReturn = "dploy_oidc_return" ) -// NewOIDCHandler discovers the IdP endpoints, wires an oauth2.Config and -// starts the state-cleanup goroutine. Returns an error only if discovery -// keeps failing after the retry budget — boot-time network races are the -// usual reason and 5 attempts with exponential backoff covers them in -// practice. +// NewOIDCHandler builds the RP from OIDC discovery. Optional split-horizon +// support: when OIDCPublicIssuer differs from OIDCIssuer, discovery is fetched +// via the in-cluster URL but the expected `iss` is the public one, and the +// browser-facing AuthURL is rebased to the public host. Single-URL setups +// leave OIDCPublicIssuer empty and the split-horizon branches are no-ops. func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { - // Split-horizon: tokens carry the public issuer URL (what the browser - // sees), but we discover and call the IdP through the in-cluster URL. - // InsecureIssuerURLContext tells go-oidc which issuer to expect. ctx := context.Background() - if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { + expectedIssuer := cfg.OIDCIssuer + splitHorizon := cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer + if splitHorizon { + // Tell go-oidc to expect tokens with iss == publicIssuer even + // though we hit the internal URL for the discovery doc. ctx = oidc.InsecureIssuerURLContext(ctx, cfg.OIDCPublicIssuer) + expectedIssuer = cfg.OIDCPublicIssuer } - provider, err := discoverWithRetry(ctx, cfg.OIDCIssuer) + provider, err := newProviderWithRetry(ctx, cfg.OIDCIssuer) if err != nil { - return nil, fmt.Errorf("failed to discover OIDC endpoints from %s: %w", cfg.OIDCIssuer, err) + return nil, fmt.Errorf("OIDC discovery against %s: %w", cfg.OIDCIssuer, err) } - logger.Info("OIDC discovery completed", "issuer", cfg.OIDCIssuer) - // Browser redirects must hit the *public* authorization endpoint; backend - // code exchange stays on the *internal* token endpoint. Discovery gave us - // both with internal URLs — substitute the public base on AuthURL. endpoint := provider.Endpoint() - authURL := endpoint.AuthURL - if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { + if splitHorizon { + // What the discovery doc advertises depends on the IdP. Dex bakes + // its configured `issuer` into every endpoint, so fetching the doc + // via the internal URL still yields *public* endpoints — nothing to + // rebase (the backend reaches the public token endpoint via + // hostAliases/DNS). IdPs that derive endpoints from the request + // Host instead return internal URLs; for those, the browser-facing + // AuthURL must be rebased onto the public host. internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) - authURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) - logger.Info("OIDC auth endpoint rebased to public", - "internal", internalBase, "public", publicBase, "authURL", authURL) + switch { + case strings.HasPrefix(endpoint.AuthURL, publicBase): + logger.Info("OIDC auth endpoint already public, no rebase needed", "authURL", endpoint.AuthURL) + case strings.HasPrefix(endpoint.AuthURL, internalBase): + endpoint.AuthURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) + logger.Info("OIDC auth endpoint rebased to public", + "internal", internalBase, "public", publicBase, "authURL", endpoint.AuthURL) + default: + logger.Warn("OIDC auth endpoint matches neither issuer base; leaving as-is", + "authURL", endpoint.AuthURL, "internal", internalBase, "public", publicBase) + } } - // Keys are random per process: an in-flight login that straddles a pod - // restart fails closed (same as the in-memory map this replaces). Stable - // keys via env/secret would survive restarts — easy follow-up if needed. - sc := securecookie.New(securecookie.GenerateRandomKey(64), securecookie.GenerateRandomKey(32)) - sc.MaxAge(int(stateTTL.Seconds())) - - return &OIDCHandler{ - config: cfg, + h := &OIDCHandler{ oauth2Config: &oauth2.Config{ ClientID: cfg.OIDCClientID, ClientSecret: cfg.OIDCClientSecret, RedirectURL: cfg.OIDCRedirectURL, + Endpoint: endpoint, Scopes: []string{oidc.ScopeOpenID, "email", "profile"}, - Endpoint: oauth2.Endpoint{ - AuthURL: authURL, // public — browser redirects here - TokenURL: endpoint.TokenURL, // internal — backend POSTs here - }, }, - sc: sc, - }, nil + verifier: provider.Verifier(&oidc.Config{ClientID: cfg.OIDCClientID}), + secureCookie: !strings.HasPrefix(cfg.OIDCRedirectURL, "http://"), + } + logger.Info("OIDC handler initialized", "expectedIssuer", expectedIssuer, "secureCookie", h.secureCookie) + return h, nil } -// discoverWithRetry rides out the post-startup network-identity window -// (Cilium et al.) where DNS / egress briefly returns EPERM. Same shape as the -// pre-refactor discoverOIDCWithRetry: 5 attempts, exponential backoff capped -// at 4 s. -func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, error) { +// newProviderWithRetry rides out the post-startup network-identity window +// (Cilium et al.) where DNS / egress briefly returns EPERM. 5 attempts with +// exponential backoff capped at 4 s. +func newProviderWithRetry(ctx context.Context, issuer string) (*oidc.Provider, error) { delay := 500 * time.Millisecond var lastErr error for i := 1; i <= discoveryAttempts; i++ { @@ -109,7 +122,7 @@ func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, erro cancel() if err == nil { if i > 1 { - logger.Info("OIDC discovery succeeded after retries", "issuer", issuer, "attempts", i) + logger.Info("OIDC discovery succeeded after retries", "attempts", i) } return provider, nil } @@ -118,7 +131,7 @@ func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, erro break } logger.Info("OIDC discovery attempt failed, retrying", - "issuer", issuer, "attempt", i, "nextDelay", delay, "error", err.Error()) + "attempt", i, "nextDelay", delay, "error", err.Error()) time.Sleep(delay) if delay < 4*time.Second { delay *= 2 @@ -127,8 +140,6 @@ func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, erro return nil, lastErr } -// extractBaseURL keeps only scheme + host (no path), so a public issuer with a -// trailing path component still substitutes cleanly. func extractBaseURL(rawURL string) string { parsed, err := url.Parse(rawURL) if err != nil { @@ -137,98 +148,177 @@ func extractBaseURL(rawURL string) string { return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host) } -// generateState signs the (returnURL, expiry) blob into a self-contained -// OAuth2 state parameter. The signing key is process-random — an attacker -// can't forge a valid blob, so the CSRF guarantee holds without any -// server-side bookkeeping. -func (h *OIDCHandler) generateState(returnURL string) (string, error) { - return h.sc.Encode("dploy-state", stateBlob{ - ReturnURL: returnURL, - Expiry: time.Now().Add(stateTTL).Unix(), - }) +// sanitizeRelativePath validates that s is a safe relative URL (no scheme, +// host, userinfo, no protocol-relative or backslash trick) and returns the +// canonical form with any user-supplied #fragment dropped. The fragment-strip +// is required because the SPA's consumeHashToken() expects the final +// redirect's hash to be exclusively "#token=..." — a leftover "/foo#section" +// would otherwise produce "/foo#section#token=..." which can't be parsed. +func sanitizeRelativePath(s string) (string, bool) { + if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { + return "", false + } + u, err := url.Parse(s) + if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { + return "", false + } + // A percent-encoded backslash (e.g. "/%5cevil.com") survives the literal + // "/\\" prefix check above but decodes into u.Path; user agents that + // normalize "\" to "/" would then read "//evil.com" as protocol-relative. + // Reject any backslash in the decoded path to close that bypass. + if strings.Contains(u.Path, "\\") { + return "", false + } + u.Fragment = "" + return u.String(), true } -// consumeState verifies the signature, decodes the blob and checks the -// embedded expiry. Replay within the TTL is theoretically possible (no -// nonce store), but mitigated by the IdP's own one-time-code semantics -// and the short TTL — acceptable for the threat model. -func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) { - var blob stateBlob - if err := h.sc.Decode("dploy-state", state, &blob); err != nil { - return nil, false - } - if time.Now().Unix() > blob.Expiry { - return nil, false +func randomURLSafe(n int) (string, error) { + b := make([]byte, n) + if _, err := rand.Read(b); err != nil { + return "", err } - return &blob, true + return base64.RawURLEncoding.EncodeToString(b), nil +} + +func (h *OIDCHandler) setFlowCookie(c *fiber.Ctx, name, value string) { + c.Cookie(&fiber.Cookie{ + Name: name, + Value: value, + Path: "/auth", + HTTPOnly: true, + Secure: h.secureCookie, + SameSite: "Lax", + MaxAge: flowCookieMaxAge, + }) } -// Login initiates the Authorization Code flow. +func (h *OIDCHandler) clearFlowCookie(c *fiber.Ctx, name string) { + c.Cookie(&fiber.Cookie{ + Name: name, + Value: "", + Path: "/auth", + HTTPOnly: true, + Secure: h.secureCookie, + SameSite: "Lax", + MaxAge: -1, + }) +} + +// Login follows the official go-oidc example: random state + PKCE verifier, +// each stored in an HttpOnly cookie, plus the AuthCodeURL redirect. The +// returnUrl piggybacks as a third cookie — no state encoding/decoding +// gymnastics needed. func (h *OIDCHandler) Login(c *fiber.Ctx) error { - returnURL := c.Query("returnUrl", "/") - // Open-redirect guard: only relative paths are accepted. - if !strings.HasPrefix(returnURL, "/") { - logger.Warn("OIDC login: invalid returnUrl, defaulting to /", "returnUrl", returnURL) - returnURL = "/" + returnURL := "/" + rawReturn := c.Query("returnUrl", "/") + if clean, ok := sanitizeRelativePath(rawReturn); ok { + returnURL = clean + } else { + logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", rawReturn) } - state, err := h.generateState(returnURL) + + state, err := randomURLSafe(24) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) } - logger.Debug("OIDC login redirect", "returnUrl", returnURL) - return c.Redirect(h.oauth2Config.AuthCodeURL(state), fiber.StatusFound) + nonce, err := randomURLSafe(16) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint nonce"}) + } + verifier := oauth2.GenerateVerifier() + + h.setFlowCookie(c, cookieState, state) + h.setFlowCookie(c, cookieVerifier, verifier) + h.setFlowCookie(c, cookieNonce, nonce) + h.setFlowCookie(c, cookieReturn, returnURL) + + return c.Redirect( + h.oauth2Config.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier), oidc.Nonce(nonce)), + fiber.StatusFound, + ) } -// Callback exchanges the code for tokens and bounces the browser to the -// stashed returnURL with the id_token in the URL hash (client-side only — -// never appears in server logs). +// Callback exchanges the code for tokens, verifies the ID token, and bounces +// the browser to returnUrl with "#token=" so the SPA can pick it up. +// CSRF protection comes from the state match (cookie vs query); replay +// protection comes from PKCE. func (h *OIDCHandler) Callback(c *fiber.Ctx) error { - if errorParam := c.Query("error"); errorParam != "" { - errorDesc := c.Query("error_description", errorParam) - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": errorDesc}) + state := c.Cookies(cookieState) + verifier := c.Cookies(cookieVerifier) + nonce := c.Cookies(cookieNonce) + returnURL := c.Cookies(cookieReturn) + // One-shot: clear cookies before any failure path so a stale flow can't + // be retried by replaying the callback URL. + h.clearFlowCookie(c, cookieState) + h.clearFlowCookie(c, cookieVerifier) + h.clearFlowCookie(c, cookieNonce) + h.clearFlowCookie(c, cookieReturn) + + // IdP-side failure (user canceled, consent denied, scope rejected, …) + // arrives as ?error=&error_description= per OAuth 2.0 + // §4.1.2.1. Surface it as a clean 400 rather than letting the request + // fall through to a misleading "state mismatch" or token-exchange error. + if idpErr := c.Query("error"); idpErr != "" { + logger.Warn("OIDC callback: IdP returned error", + "error", idpErr, "description", c.Query("error_description")) + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{ + "error": "identity provider rejected the login: " + idpErr, + "error_description": c.Query("error_description"), + }) } - code := c.Query("code") - state := c.Query("state") - if code == "" || state == "" { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing code or state parameter"}) + + if state == "" || verifier == "" || nonce == "" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing or expired login session"}) } - stateData, valid := h.consumeState(state) - if !valid { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid or expired state parameter"}) + if subtle.ConstantTimeCompare([]byte(c.Query("state")), []byte(state)) != 1 { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "state mismatch"}) + } + code := c.Query("code") + if code == "" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing authorization code"}) } - token, err := h.oauth2Config.Exchange(c.Context(), code) + token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(verifier)) if err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ - "error": fmt.Sprintf("failed to exchange code: %v", err), - }) + // Detail stays server-side: oauth2 errors embed the token endpoint + // URL, which would map internal cluster topology for the caller. + logger.Error("OIDC callback: token exchange failed", "error", err) + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed"}) } - - // OIDC flows put the id_token in Token.Extra; OAuth2-only IdPs only return - // an access_token. We prefer the id_token (it's what the JWT validator - // downstream expects), fall back to the access_token otherwise. - tokenToUse, _ := token.Extra("id_token").(string) - if tokenToUse == "" { - tokenToUse = token.AccessToken + rawIDToken, ok := token.Extra("id_token").(string) + if !ok { + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "no id_token in token response"}) } - if tokenToUse == "" { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ - "error": "no token received from OIDC provider", - }) + idToken, err := h.verifier.Verify(c.Context(), rawIDToken) + if err != nil { + logger.Error("OIDC callback: id_token verification failed", "error", err) + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed"}) + } + // Nonce binds this ID token to *our* login attempt: even if both the + // auth code and PKCE verifier are intercepted, the IdP would return a + // token carrying the attacker's nonce, not ours. Constant-time compare + // because the nonce stays a one-shot secret until the cookie clears. + if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 { + // Client-side/session issue or an attack, not an upstream failure — + // mirror the state-mismatch path with a 400 rather than a 502. + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"}) } - returnURL := "/" - if stateData != nil && strings.HasPrefix(stateData.ReturnURL, "/") { - returnURL = stateData.ReturnURL + // Re-sanitize the returnUrl from the cookie as defense-in-depth, even + // though Login already vetted it before setting the cookie. + if clean, ok := sanitizeRelativePath(returnURL); ok { + returnURL = clean + } else { + returnURL = "/" } - logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokenToUse)) - return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, tokenToUse), fiber.StatusFound) + logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) + return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) } -// Logout currently just bounces the browser home — the SPA clears its -// localStorage token on this redirect. End-session at the IdP is opt-in -// (the previous version didn't do it either; add an EndSessionEndpoint -// roundtrip here when an IdP demands SLO). +// Logout bounces home — the SPA clears its localStorage token on the +// redirect. RP-initiated logout against the IdP would be an extra round-trip; +// add it when an IdP requires SLO. func (h *OIDCHandler) Logout(c *fiber.Ctx) error { return c.Redirect("/", fiber.StatusFound) } diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go new file mode 100644 index 0000000..f920e8b --- /dev/null +++ b/internal/auth/oidc_test.go @@ -0,0 +1,63 @@ +package auth + +import "testing" + +// TestSanitizeRelativePath pins both behaviors in one go: the relative-URL +// safety check (open-redirect surface) and the fragment-stripping canonical +// form the SPA's consumeHashToken() relies on. Both come out of one +// net/url.Parse pass — no string fiddling. +func TestSanitizeRelativePath(t *testing.T) { + for _, tc := range []struct { + in string + want string + wantOK bool + }{ + // happy paths — exact passthrough or fragment dropped + {"/", "/", true}, + {"/foo", "/foo", true}, + {"/foo?x=1", "/foo?x=1", true}, + {"/foo?x=1#frag", "/foo?x=1", true}, // fragment stripped + {"/foo#frag", "/foo", true}, + {"/#frag", "/", true}, + + // classic open-redirect tricks — all rejected + {"//evil.com/x", "", false}, // protocol-relative URL + {"/\\evil.com/x", "", false}, // backslash-prefixed (some browsers) + {"/%5cevil.com/x", "", false}, // percent-encoded backslash + {"/%5Cevil.com/x", "", false}, // percent-encoded backslash (upper) + {"/foo/%5cbar", "", false}, // encoded backslash mid-path + {"http://evil.com/x", "", false}, // absolute URL + {"https://evil.com/x", "", false}, // absolute URL https + {"javascript:alert(1)", "", false}, + {"data:text/html,x", "", false}, + {"//user@evil.com/x", "", false}, // userinfo trick + {"", "", false}, // empty + {"foo/bar", "", false}, // no leading slash + {" /foo", "", false}, // leading whitespace + } { + got, ok := sanitizeRelativePath(tc.in) + if ok != tc.wantOK || got != tc.want { + t.Errorf("sanitizeRelativePath(%q) = (%q, %v), want (%q, %v)", tc.in, got, ok, tc.want, tc.wantOK) + } + } +} + +// TestExtractBaseURL guards the split-horizon AuthURL rebase: if the helper +// strips path/query as expected, strings.Replace(authURL, internalBase, ...) +// hits exactly one match. The test cases mirror the two real-world Issuers +// (in-cluster Service URL with port, public URL via ingress). +func TestExtractBaseURL(t *testing.T) { + for _, tc := range []struct { + in, want string + }{ + {"http://dex.dex.svc.cluster.local:5556", "http://dex.dex.svc.cluster.local:5556"}, + {"http://dex.dex.svc.cluster.local:5556/", "http://dex.dex.svc.cluster.local:5556"}, + {"https://dex.dploy.ctf.local", "https://dex.dploy.ctf.local"}, + {"https://dex.dploy.ctf.local/dex", "https://dex.dploy.ctf.local"}, + {"https://dex.dploy.ctf.local/dex/auth?x=1", "https://dex.dploy.ctf.local"}, + } { + if got := extractBaseURL(tc.in); got != tc.want { + t.Errorf("extractBaseURL(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/internal/controller/dployinstance_controller.go b/internal/controller/dployinstance_controller.go index 1bd9799..af08995 100644 --- a/internal/controller/dployinstance_controller.go +++ b/internal/controller/dployinstance_controller.go @@ -301,15 +301,15 @@ func (r *DployInstanceReconciler) buildData(inst *dployv1alpha1.DployInstance, t } return &templating.Data{ - Owner: sanitize(owner), - UUID: inst.Status.UUID, - BaseDomain: eff.BaseDomain, - Host: defaultHost(inst.Spec.TemplateRef, inst.Status.UUID, eff.BaseDomain), - Namespace: targetNS, - Template: tmpl, - Params: params, - Claims: claims, - Config: templating.Config{Values: eff.Values}, + Owner: sanitize(owner), + UUID: inst.Status.UUID, + BaseDomain: eff.BaseDomain, + Host: defaultHost(inst.Spec.TemplateRef, inst.Status.UUID, eff.BaseDomain), + Namespace: targetNS, + Template: tmpl, + Params: params, + Claims: claims, + Config: templating.Config{Values: eff.Values}, }, nil } diff --git a/internal/controller/flux.go b/internal/controller/flux.go index 543e4b1..b702dda 100644 --- a/internal/controller/flux.go +++ b/internal/controller/flux.go @@ -12,8 +12,8 @@ import ( fluxmeta "github.com/fluxcd/pkg/apis/meta" sourcev1 "github.com/fluxcd/source-controller/api/v1" corev1 "k8s.io/api/core/v1" - apimeta "k8s.io/apimachinery/pkg/api/meta" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" diff --git a/internal/kube/client.go b/internal/kube/client.go index ed82f64..4094112 100644 --- a/internal/kube/client.go +++ b/internal/kube/client.go @@ -284,7 +284,7 @@ func (c *Client) ExtendInstance(ctx context.Context, inst *dployv1alpha1.DployIn return time.Time{}, fmt.Errorf("%w (%d)", ErrMaxExtends, maxExtends) } - newExpires := inst.Spec.ExpiresAt.Time.Add(time.Duration(extendSeconds) * time.Second) + newExpires := inst.Spec.ExpiresAt.Add(time.Duration(extendSeconds) * time.Second) patch := client.MergeFrom(inst.DeepCopy()) t := metav1.NewTime(newExpires) inst.Spec.ExpiresAt = &t diff --git a/internal/models/responses.go b/internal/models/responses.go index 0d42a8f..948926b 100644 --- a/internal/models/responses.go +++ b/internal/models/responses.go @@ -6,10 +6,10 @@ type AvailableEnvironmentResponse struct { Icon string `json:"icon"` Category string `json:"category,omitempty"` // TTL info - TTL int `json:"ttl"` // Initial TTL in seconds (-1 for unlimited) - ExtendTTL int `json:"extendTTL,omitempty"` // Seconds added per extension (0 = use default) - MaxExtends int `json:"maxExtends,omitempty"` // Max extensions allowed (0 = unlimited) - IsUnlimited bool `json:"isUnlimited"` // True if TTL is unlimited + TTL int `json:"ttl"` // Initial TTL in seconds (-1 for unlimited) + ExtendTTL int `json:"extendTTL,omitempty"` // Seconds added per extension (0 = use default) + MaxExtends int `json:"maxExtends,omitempty"` // Max extensions allowed (0 = unlimited) + IsUnlimited bool `json:"isUnlimited"` // True if TTL is unlimited } type UserEnvironmentResponse struct { diff --git a/internal/operatorconfig/resolver.go b/internal/operatorconfig/resolver.go index 468e831..36e051e 100644 --- a/internal/operatorconfig/resolver.go +++ b/internal/operatorconfig/resolver.go @@ -29,19 +29,19 @@ const ( // Effective is the merged, ready-to-use operator configuration. type Effective struct { - DefaultEngine dployv1alpha1.EngineType - FluxNamespace string - FluxServiceAccount string - FluxInterval time.Duration + DefaultEngine dployv1alpha1.EngineType + FluxNamespace string + FluxServiceAccount string + FluxInterval time.Duration BaseDomain string ConnectionURLTemplate string DefaultConnectionType dployv1alpha1.ConnectionType ConnectionMessageTemplate string TTLSeconds int64 - ExtendSeconds int64 - MaxExtends int - MaxInstancesPerUser int - Values map[string]any + ExtendSeconds int64 + MaxExtends int + MaxInstancesPerUser int + Values map[string]any } // Resolve reads the OperatorConfig named "default" and merges it over the