From 997f9a8cafe05b179b8063308a65c9fd041688ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Apr 2026 01:10:08 +0200 Subject: [PATCH 01/15] =?UTF-8?q?fix(security/gcp):=20drop=20plaintext=20S?= =?UTF-8?q?CHEDULED=5FTASK=5FSECRET=20=E2=80=94=20rely=20on=20OIDC=20+=20C?= =?UTF-8?q?loud=20Run=20IAM?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #159 (companion to #50, which fixed Azure). Cloud Scheduler jobs in three sites embedded the shared scheduled-task secret as a plaintext `Authorization: Bearer ${var.scheduled_task_secret}` header. The secret therefore lived in: - The Cloud Scheduler resource definition stored in GCP. - Terraform state (any var.* value flowing into a resource attribute is captured in state). - `terraform plan` / `terraform show` output captured in CI logs and operator screens. Cloud Run was already fronted by `roles/run.invoker` (granted to the scheduler's user-assigned service account) and `oidc_token` (signed at invocation), so the network-level + IAM-level auth was already in place. The static bearer header was redundant defense-in-depth — and the one reason the plaintext secret had to flow into the IaC at all. Fix: drop the static `Authorization` header from all three scheduler resources. Cloud Run validates the OIDC token via its IAM gate (`cloud_run_allow_unauthenticated = false`, the default per #51); the application-level bearer-check at `internal/server/http.go:117` already no-ops when `SCHEDULED_TASK_SECRET` is empty, so dropping the env var from Cloud Run + GKE makes that check skip cleanly on GCP. Side cleanup: - Removed `var.scheduled_task_secret` from cloud-run + gke module variable declarations (no longer referenced). - Removed `scheduled_task_secret = module.secrets.scheduled_task_secret_value` from the cloud-run module call in `terraform/environments/gcp/compute.tf`. - Removed `SCHEDULED_TASK_SECRET = module.secrets.scheduled_task_secret_value` from the Cloud Run + GKE additional_env_vars maps. - The GCP secrets module still creates the random_password + google_secret_manager_secret resources behind `create_scheduled_task_secret` so callers can flip that flag off in tfvars; left intact to avoid unrelated state-removal in this PR. Azure path unchanged. Azure Container Apps lacks Cloud Run's built-in OIDC validation, so the bearer scheme + Key Vault runtime fetch (per #50, already shipped) stays as the application-level auth on Azure. Validation: - `terraform validate` clean for cloud-run, gke, and the gcp environment. - `grep -rn "Bearer \\${var.scheduled_task_secret}" terraform/` — no matches. The remaining `scheduled_task_secret_*` references in `terraform/modules/secrets/{gcp,azure}/` are the secret-creation resources themselves and the Azure-side wiring (which uses _NAME + runtime fetch). --- terraform/environments/gcp/compute.tf | 10 ++++++---- terraform/modules/compute/gcp/cloud-run/main.tf | 17 +++++++++-------- .../modules/compute/gcp/cloud-run/variables.tf | 7 ------- terraform/modules/compute/gcp/gke/main.tf | 11 +++++++---- terraform/modules/compute/gcp/gke/variables.tf | 7 ------- 5 files changed, 22 insertions(+), 30 deletions(-) diff --git a/terraform/environments/gcp/compute.tf b/terraform/environments/gcp/compute.tf index 38219ded8..644c4b64d 100644 --- a/terraform/environments/gcp/compute.tf +++ b/terraform/environments/gcp/compute.tf @@ -49,10 +49,14 @@ module "compute_cloud_run" { # VPC Access (for Cloud SQL) vpc_connector_id = module.networking.vpc_connector_id - # Scheduled tasks + # Scheduled tasks. Per #159 the cloud-run module no longer takes a + # plaintext scheduled_task_secret — Cloud Scheduler authenticates to + # Cloud Run via OIDC token + roles/run.invoker. Application-level + # bearer auth is dropped on GCP (Cloud Run's IAM gate is the + # primary defence; on Azure the bearer check stays because Container + # Apps does not have Cloud Run's built-in OIDC validation). enable_scheduled_tasks = var.enable_scheduled_tasks recommendation_schedule = var.recommendation_schedule - scheduled_task_secret = module.secrets.scheduled_task_secret_value # RI exchange automation enable_ri_exchange_schedule = var.enable_ri_exchange_schedule @@ -73,7 +77,6 @@ module "compute_cloud_run" { FROM_EMAIL = var.subdomain_zone_name != "" ? "noreply@${var.subdomain_zone_name}" : "noreply@${var.project_name}.example.com" DASHBOARD_URL = local.dashboard_url CORS_ALLOWED_ORIGIN = local.dashboard_url != "" ? local.dashboard_url : "http://localhost:3000" - SCHEDULED_TASK_SECRET = module.secrets.scheduled_task_secret_value CUDLY_MAX_ACCOUNT_PARALLELISM = tostring(var.max_account_parallelism) CUDLY_SOURCE_CLOUD = "gcp" }, @@ -147,7 +150,6 @@ module "compute_gke" { FROM_EMAIL = var.subdomain_zone_name != "" ? "noreply@${var.subdomain_zone_name}" : "noreply@${var.project_name}.example.com" DASHBOARD_URL = local.dashboard_url CORS_ALLOWED_ORIGIN = local.dashboard_url != "" ? local.dashboard_url : "http://localhost:3000" - SCHEDULED_TASK_SECRET = module.secrets.scheduled_task_secret_value CUDLY_MAX_ACCOUNT_PARALLELISM = tostring(var.max_account_parallelism) CUDLY_SOURCE_CLOUD = "gcp" }, diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 40802dfdc..5685a7eb1 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -269,10 +269,13 @@ resource "google_cloud_scheduler_job" "recommendations" { http_method = "POST" uri = "${google_cloud_run_v2_service.main.uri}/api/scheduled/recommendations" - headers = { - "Authorization" = "Bearer ${var.scheduled_task_secret}" - } - + # Auth: oidc_token below is signed by the scheduler's service + # account at invocation time and validated by Cloud Run's IAM gate + # (roles/run.invoker grants this SA access; cloud_run_allow_unauthenticated + # defaults to false). The previous static `Authorization: Bearer + # ${var.scheduled_task_secret}` header leaked the shared secret into + # the scheduler resource definition + Terraform state — closes #159. + # OIDC supersedes the application-level bearer check on GCP. oidc_token { service_account_email = google_service_account.scheduler[0].email } @@ -322,10 +325,8 @@ resource "google_cloud_scheduler_job" "ri_exchange" { http_method = "POST" uri = "${google_cloud_run_v2_service.main.uri}/api/scheduled/ri-exchange" - headers = { - "Authorization" = "Bearer ${var.scheduled_task_secret}" - } - + # Same OIDC-only model as the recommendations scheduler above — + # see #159 for the rationale. oidc_token { service_account_email = google_service_account.scheduler_ri_exchange[0].email } diff --git a/terraform/modules/compute/gcp/cloud-run/variables.tf b/terraform/modules/compute/gcp/cloud-run/variables.tf index c4adcea19..6da6d4bf6 100644 --- a/terraform/modules/compute/gcp/cloud-run/variables.tf +++ b/terraform/modules/compute/gcp/cloud-run/variables.tf @@ -161,13 +161,6 @@ variable "recommendation_schedule" { default = "0 2 * * *" } -variable "scheduled_task_secret" { - description = "Shared secret for authenticating scheduled task HTTP calls" - type = string - default = "" - sensitive = true -} - variable "additional_secret_accessor_ids" { description = <<-EOT Map of Secret Manager secret IDs (full resource names) the Cloud Run diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 61b475565..8658b85ad 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -672,10 +672,13 @@ resource "google_cloud_scheduler_job" "recommendations" { http_method = "POST" uri = "${var.app_url}/api/scheduled/recommendations" - headers = { - "Authorization" = "Bearer ${var.scheduled_task_secret}" - } - + # Auth: oidc_token below is signed by the scheduler's service account + # at invocation time. The previous static `Authorization: Bearer + # ${var.scheduled_task_secret}` header leaked the shared secret into + # the scheduler resource definition + Terraform state — closes #159. + # OIDC supersedes the application-level bearer check on GCP. (The + # GKE-side var was also defaulting to "" in practice — the static + # header was sending an empty Bearer header anyway.) oidc_token { service_account_email = google_service_account.scheduler[0].email } diff --git a/terraform/modules/compute/gcp/gke/variables.tf b/terraform/modules/compute/gcp/gke/variables.tf index 38ef98b3a..1331f5fa2 100644 --- a/terraform/modules/compute/gcp/gke/variables.tf +++ b/terraform/modules/compute/gcp/gke/variables.tf @@ -261,13 +261,6 @@ variable "recommendation_schedule" { default = "0 2 * * *" } -variable "scheduled_task_secret" { - description = "Shared secret for authenticating scheduled task HTTP calls" - type = string - default = "" - sensitive = true -} - variable "app_url" { description = "Application URL for scheduled task HTTP triggers (e.g., https://app.example.com). Required when enable_scheduled_tasks is true." type = string From 9caee8b3e8aaf69d1f879d96149712b7950b6579 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 20:41:29 +0200 Subject: [PATCH 02/15] feat(server): add scheduled-task OIDC validator (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new `internal/server/scheduledauth` package providing app-level authentication for the `/api/scheduled/*` endpoints. Three modes: - `oidc` — verifies a Google-issued OIDC ID token (RS256, JWKS- validated). Subject pinning to the scheduler SA email is REQUIRED (defence in depth — any GCP SA in the org can mint a token with arbitrary `aud`, only the scheduler has our `sub`). 60s clock skew, refresh-on-unknown-kid for key rotation, single-flight JWKS cache via go-oidc/v3. - `bearer` — constant-time compare against a shared secret. Used on Azure where Logic Apps fetch the secret from Key Vault at invocation time. - `disabled` — local dev only. Loud per-request WARN logs. Mode is selected via `SCHEDULED_TASK_AUTH_MODE`; OIDC params via `SCHEDULED_TASK_OIDC_AUDIENCE` (CSV) and `SCHEDULED_TASK_OIDC_SUBJECTS` (CSV, REQUIRED in oidc mode); bearer secret via `SCHEDULED_TASK_SECRET`. Fail-fast on misconfig — empty subjects or unknown mode returns `ErrConfigInvalid` so the server refuses to start rather than silently accepting everything. This is the validator + config parser only; wiring into the HTTP server and the Terraform changes follow in subsequent commits. Closes the auth gap surfaced during PR #161 verification: dropping the plaintext `SCHEDULED_TASK_SECRET` from Cloud Scheduler headers leaves `/api/scheduled/*` unauthenticated on Cloud Run / GKE (both have `cloud_run_allow_unauthenticated = true` today; #78 will flip that separately as defence-in-depth, but Option B is independent of it). --- go.mod | 3 +- go.sum | 6 +- internal/server/scheduledauth/config.go | 82 ++ internal/server/scheduledauth/errors.go | 29 + internal/server/scheduledauth/validator.go | 388 ++++++++++ .../server/scheduledauth/validator_test.go | 717 ++++++++++++++++++ 6 files changed, 1222 insertions(+), 3 deletions(-) create mode 100644 internal/server/scheduledauth/config.go create mode 100644 internal/server/scheduledauth/errors.go create mode 100644 internal/server/scheduledauth/validator.go create mode 100644 internal/server/scheduledauth/validator_test.go diff --git a/go.mod b/go.mod index 8fd3bfa3c..7425aa134 100644 --- a/go.mod +++ b/go.mod @@ -103,6 +103,8 @@ require ( github.com/aws/aws-sdk-go-v2/service/sesv2 v1.42.0 github.com/aws/aws-sdk-go-v2/service/sns v1.34.0 github.com/aws/aws-sdk-go-v2/service/sts v1.26.6 + github.com/coreos/go-oidc/v3 v3.18.0 + github.com/go-jose/go-jose/v4 v4.1.4 github.com/golang-migrate/migrate/v4 v4.19.1 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.8.0 @@ -148,7 +150,6 @@ require ( github.com/ebitengine/purego v0.8.4 // indirect github.com/envoyproxy/go-control-plane/envoy v1.36.0 // indirect github.com/envoyproxy/protoc-gen-validate v1.3.0 // indirect - github.com/go-jose/go-jose/v4 v4.1.3 // indirect github.com/go-ole/go-ole v1.2.6 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.6 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect diff --git a/go.sum b/go.sum index 09f5bcae7..41a86df56 100644 --- a/go.sum +++ b/go.sum @@ -174,6 +174,8 @@ github.com/containerd/log v0.1.0 h1:TCJt7ioM2cr/tfR8GPbGf9/VRAX8D2B4PjzCpfX540I= github.com/containerd/log v0.1.0/go.mod h1:VRRf09a7mHDIRezVKTRCrOq78v577GXq3bSa3EhrzVo= github.com/containerd/platforms v0.2.1 h1:zvwtM3rz2YHPQsF2CHYM8+KtB5dvhISiXh5ZpSBQv6A= github.com/containerd/platforms v0.2.1/go.mod h1:XHCb+2/hzowdiut9rkudds9bE5yJ7npe7dG/wG+uFPw= +github.com/coreos/go-oidc/v3 v3.18.0 h1:V9orjXynvu5wiC9SemFTWnG4F45v403aIcjWo0d41+A= +github.com/coreos/go-oidc/v3 v3.18.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= @@ -207,8 +209,8 @@ github.com/envoyproxy/protoc-gen-validate v1.3.0 h1:TvGH1wof4H33rezVKWSpqKz5NXWg github.com/envoyproxy/protoc-gen-validate v1.3.0/go.mod h1:HvYl7zwPa5mffgyeTUHA9zHIH36nmrm7oCbo4YKoSWA= github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg= github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= -github.com/go-jose/go-jose/v4 v4.1.3 h1:CVLmWDhDVRa6Mi/IgCgaopNosCaHz7zrMeF9MlZRkrs= -github.com/go-jose/go-jose/v4 v4.1.3/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= +github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= +github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= diff --git a/internal/server/scheduledauth/config.go b/internal/server/scheduledauth/config.go new file mode 100644 index 000000000..d1561d2c9 --- /dev/null +++ b/internal/server/scheduledauth/config.go @@ -0,0 +1,82 @@ +package scheduledauth + +import ( + "fmt" + "strings" +) + +// EnvSource is the minimal interface NewFromEnv depends on. The default +// implementation (envOS) reads from os.Getenv; tests inject a map. +type EnvSource interface { + Get(string) string +} + +// EnvMap is an EnvSource backed by a map. Useful in tests. +type EnvMap map[string]string + +// Get returns the value for key, or "" if absent. +func (m EnvMap) Get(key string) string { return m[key] } + +// Environment variable names. Centralised so tests and Terraform stay +// aligned with the Go code. +const ( + EnvAuthMode = "SCHEDULED_TASK_AUTH_MODE" // "oidc" | "bearer" | "disabled" + EnvOIDCIssuer = "SCHEDULED_TASK_OIDC_ISSUER" // default GoogleIssuer + EnvOIDCJWKSURL = "SCHEDULED_TASK_OIDC_JWKS_URL" // default GoogleJWKSURL + EnvOIDCAudience = "SCHEDULED_TASK_OIDC_AUDIENCE" // comma-separated + EnvOIDCSubjects = "SCHEDULED_TASK_OIDC_SUBJECTS" // comma-separated; REQUIRED in oidc mode + EnvBearerSecret = "SCHEDULED_TASK_SECRET" // bearer mode only +) + +// LoadConfig parses Config from env. Unset SCHEDULED_TASK_AUTH_MODE +// defaults to ModeDisabled (with a startup WARN logged inside New). Any +// other invalid combination returns an ErrConfigInvalid error so the +// server fails fast instead of silently downgrading to no-auth. +func LoadConfig(env EnvSource) (Config, error) { + mode := strings.ToLower(strings.TrimSpace(env.Get(EnvAuthMode))) + if mode == "" { + mode = string(ModeDisabled) + } + + cfg := Config{Mode: Mode(mode)} + + switch cfg.Mode { + case ModeOIDC: + cfg.Issuer = strings.TrimSpace(env.Get(EnvOIDCIssuer)) + cfg.JWKSURL = strings.TrimSpace(env.Get(EnvOIDCJWKSURL)) + cfg.Audiences = splitCSV(env.Get(EnvOIDCAudience)) + cfg.Subjects = splitCSV(env.Get(EnvOIDCSubjects)) + + case ModeBearer: + cfg.Bearer = env.Get(EnvBearerSecret) + // Permit other env vars to be set without complaint — the + // terraform module ships SCHEDULED_TASK_OIDC_* across all + // platforms and they should be ignored on Azure. + + case ModeDisabled: + // Nothing to parse. + + default: + return Config{}, fmt.Errorf("%w: %s=%q (expected oidc, bearer, or disabled)", + ErrConfigInvalid, EnvAuthMode, mode) + } + + return cfg, nil +} + +// splitCSV splits a comma-separated value into a trimmed list, dropping +// empty entries (so trailing commas / whitespace are tolerated). +func splitCSV(raw string) []string { + if raw == "" { + return nil + } + parts := strings.Split(raw, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + p = strings.TrimSpace(p) + if p != "" { + out = append(out, p) + } + } + return out +} diff --git a/internal/server/scheduledauth/errors.go b/internal/server/scheduledauth/errors.go new file mode 100644 index 000000000..b1624b0d8 --- /dev/null +++ b/internal/server/scheduledauth/errors.go @@ -0,0 +1,29 @@ +// Package scheduledauth provides authentication for the /api/scheduled/* +// endpoints invoked by Cloud Scheduler / Logic Apps. Two modes are +// supported: +// +// - oidc: Google-issued OIDC ID tokens (RS256, JWKS-validated). Used +// by GCP Cloud Run and GKE deployments where Cloud Scheduler signs an +// OIDC token with a service-account audience. Subject pinning to the +// scheduler service account email(s) is REQUIRED — the audience alone +// is not sufficient if other tenants in the same GCP org could mint +// tokens with the same `aud` value. +// +// - bearer: shared-secret HMAC-style bearer (constant-time compare). +// Used by Azure Container Apps + Logic Apps where the workflow +// pulls the secret from Key Vault at invocation time. +// +// See plan: ~/.claude/projects/.../plans/issue-161-oidc-validator.md +package scheduledauth + +import "errors" + +// ErrConfigInvalid signals a fatal configuration parse error. Callers are +// expected to fail-fast on this — the deploy is misconfigured and the +// scheduled endpoint would silently accept or reject everything. +var ErrConfigInvalid = errors.New("scheduledauth: invalid configuration") + +// ErrUnauthorized is the validation failure returned by Validate. The +// detailed reason is wrapped via fmt.Errorf so it gets logged but not +// returned to the client (which only sees 401). +var ErrUnauthorized = errors.New("scheduledauth: unauthorized") diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go new file mode 100644 index 000000000..c2102c73d --- /dev/null +++ b/internal/server/scheduledauth/validator.go @@ -0,0 +1,388 @@ +package scheduledauth + +import ( + "context" + "crypto/subtle" + "errors" + "fmt" + "log" + "net/http" + "strings" + "time" + + "github.com/coreos/go-oidc/v3/oidc" +) + +// Mode is the authentication mode used for scheduled-task requests. +type Mode string + +const ( + // ModeOIDC verifies a Google-issued OIDC ID token (RS256 only) on + // the Authorization header. Subject pinning is required. + ModeOIDC Mode = "oidc" + + // ModeBearer compares the Authorization header to a shared secret + // using crypto/subtle.ConstantTimeCompare. Used on Azure where Logic + // Apps fetch the secret from Key Vault at runtime. + ModeBearer Mode = "bearer" + + // ModeDisabled disables auth entirely — every request is allowed + // through. Intended for local development only. The validator logs + // a WARN at startup and on every request to make this loud. + ModeDisabled Mode = "disabled" +) + +// DefaultClockSkew is the tolerance applied to exp/nbf checks. Matches +// the leeway most OIDC providers (Microsoft, Auth0) recommend. +const DefaultClockSkew = 60 * time.Second + +// GoogleIssuer is the canonical OIDC issuer URL for Google-signed ID +// tokens (Cloud Scheduler, Cloud Functions, etc.). +const GoogleIssuer = "https://accounts.google.com" + +// GoogleJWKSURL is Google's JWKS endpoint. Used as the default when the +// caller does not override it via SCHEDULED_TASK_OIDC_JWKS_URL. +const GoogleJWKSURL = "https://www.googleapis.com/oauth2/v3/certs" + +// Config holds the parsed configuration for a Validator. NewValidator +// applies defaults and rejects invalid combinations. +type Config struct { + Mode Mode + Issuer string // OIDC issuer; defaults to GoogleIssuer in oidc mode + JWKSURL string // OIDC JWKS endpoint; defaults to GoogleJWKSURL in oidc mode + Audiences []string // accepted aud claims (must be non-empty in oidc mode) + Subjects []string // accepted sub claims (REQUIRED non-empty in oidc mode — defence in depth) + Skew time.Duration + Bearer string // shared secret for bearer mode (must be non-empty) +} + +// Validator authenticates inbound requests against the configured mode. +type Validator struct { + mode Mode + verifier *oidc.IDTokenVerifier // nil unless mode == ModeOIDC + keySet *oidc.RemoteKeySet // nil unless mode == ModeOIDC; powered by go-oidc's single-flight cache + jwksURL string // remembered for Warmup; empty unless mode == ModeOIDC + audiences map[string]struct{} + subjects map[string]struct{} + skew time.Duration + bearer []byte + now func() time.Time // pluggable for tests +} + +// claims captures the timestamp claims we need beyond what go-oidc's +// IDToken exposes directly. Issuer/Subject/Audience are read off the +// IDToken value returned by Verify; we only re-parse the payload to +// pick up exp/iat/nbf as int64 timestamps so we can apply our own +// skew-tolerant checks (go-oidc's built-in expiry check has no skew). +type claims struct { + ExpiresAt int64 `json:"exp"` + IssuedAt int64 `json:"iat"` + NotBefore int64 `json:"nbf"` +} + +// New builds a Validator from a parsed Config. The caller is responsible +// for invoking NewFromEnv (or a similar config loader) to populate +// Config from environment variables. +// +// In oidc mode, the underlying *oidc.RemoteKeySet performs JWKS retrieval +// lazily on the first request and refreshes on unknown kid (per +// https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys). +// Single-flight is built in. There is no fixed TTL — refresh-on- +// unknown-kid is sufficient because providers always sign new tokens +// with the new kid before retiring the old key. +// +// To pre-warm the cache and surface JWKS endpoint misconfiguration at +// startup, callers should invoke (*Validator).Warmup. Failure is logged +// but does not abort startup — Google's CDN sometimes hiccups and we +// prefer the validator come up with a stale-on-error fetch on the first +// real request rather than crashloop the container. +func New(cfg Config) (*Validator, error) { + if cfg.Skew <= 0 { + cfg.Skew = DefaultClockSkew + } + + v := &Validator{ + mode: cfg.Mode, + skew: cfg.Skew, + now: time.Now, + } + + switch cfg.Mode { + case ModeOIDC: + return configureOIDC(v, cfg) + case ModeBearer: + return configureBearer(v, cfg) + case ModeDisabled: + // No setup; every request passes (with a per-request WARN log). + log.Printf("scheduledauth: WARN — auth mode is disabled; /api/scheduled/* is unauthenticated. " + + "Set SCHEDULED_TASK_AUTH_MODE=oidc (GCP) or =bearer (Azure) in production.") + return v, nil + default: + return nil, fmt.Errorf("%w: unknown mode %q (expected oidc, bearer, or disabled)", ErrConfigInvalid, cfg.Mode) + } +} + +// configureOIDC validates OIDC config, builds set-lookups, and wires the +// go-oidc RemoteKeySet + IDTokenVerifier. Split out of New to keep the +// per-mode setup below the cyclomatic-complexity gate. +func configureOIDC(v *Validator, cfg Config) (*Validator, error) { + if cfg.Issuer == "" { + cfg.Issuer = GoogleIssuer + } + if cfg.JWKSURL == "" { + cfg.JWKSURL = GoogleJWKSURL + } + if len(cfg.Audiences) == 0 { + return nil, fmt.Errorf("%w: oidc mode requires SCHEDULED_TASK_OIDC_AUDIENCE", ErrConfigInvalid) + } + // Subject pinning is REQUIRED in oidc mode. Any GCP service account + // in the same org can mint OIDC tokens with arbitrary `aud` values + // — we MUST also pin the issuer SA email via `sub`. See plan §1. + if len(cfg.Subjects) == 0 { + return nil, fmt.Errorf("%w: oidc mode requires SCHEDULED_TASK_OIDC_SUBJECTS (defence in depth)", ErrConfigInvalid) + } + + auds, err := cleanSet(cfg.Audiences, "SCHEDULED_TASK_OIDC_AUDIENCE") + if err != nil { + return nil, err + } + subs, err := cleanSet(cfg.Subjects, "SCHEDULED_TASK_OIDC_SUBJECTS") + if err != nil { + return nil, err + } + + v.audiences = auds + v.subjects = subs + v.jwksURL = cfg.JWKSURL + v.keySet = oidc.NewRemoteKeySet(context.Background(), cfg.JWKSURL) + v.verifier = oidc.NewVerifier(cfg.Issuer, v.keySet, &oidc.Config{ + // Pin to RS256. Google's tokens are RS256; rejecting anything + // else closes the alg=none / alg=HS256 confusion family. + SupportedSigningAlgs: []string{string(oidc.RS256)}, + // We validate audience manually (multiple values supported) + // and expiry manually (skew tolerance). Issuer is checked by + // go-oidc. + SkipClientIDCheck: true, + SkipExpiryCheck: true, + }) + return v, nil +} + +func configureBearer(v *Validator, cfg Config) (*Validator, error) { + if cfg.Bearer == "" { + return nil, fmt.Errorf("%w: bearer mode requires SCHEDULED_TASK_SECRET", ErrConfigInvalid) + } + v.bearer = []byte("Bearer " + cfg.Bearer) + return v, nil +} + +// cleanSet trims and deduplicates input, rejecting blank entries that +// would silently match a missing claim. +func cleanSet(in []string, label string) (map[string]struct{}, error) { + out := make(map[string]struct{}, len(in)) + for _, s := range in { + s = strings.TrimSpace(s) + if s == "" { + return nil, fmt.Errorf("%w: %s contains empty value", ErrConfigInvalid, label) + } + out[s] = struct{}{} + } + return out, nil +} + +// Mode returns the validator's auth mode. Useful for /health. +func (v *Validator) Mode() Mode { + return v.mode +} + +// Warmup performs a best-effort sanity check on the JWKS endpoint at +// startup. Failure is non-fatal — it is logged and the validator stays +// operable; the underlying *oidc.RemoteKeySet retries on the first real +// request. This avoids crashlooping the container when Google's CDN +// hiccups, while still surfacing misconfiguration (wrong URL, blocked +// egress) prominently in the startup logs. +// +// Implementation note: go-oidc's RemoteKeySet does not expose a public +// "fetch now" method — it only fetches on demand from VerifySignature +// (which requires a parseable JWT). We issue an independent HTTP GET +// to the JWKS URL to keep the warmup probe lightweight and reusable. +func (v *Validator) Warmup(ctx context.Context) { + if v.mode != ModeOIDC || v.jwksURL == "" { + return + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, v.jwksURL, nil) + if err != nil { + log.Printf("scheduledauth: WARN — JWKS warmup request build failed: %v", err) + return + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + log.Printf("scheduledauth: WARN — JWKS warmup fetch failed for %s: %v "+ + "(validator will retry on first request)", v.jwksURL, err) + return + } + defer resp.Body.Close() + if resp.StatusCode >= 400 { + log.Printf("scheduledauth: WARN — JWKS warmup got %d from %s "+ + "(validator will retry on first request)", resp.StatusCode, v.jwksURL) + } +} + +// Middleware wraps next with auth enforcement. On failure, writes a 401 +// with no body (avoiding any implementation-detail leakage) and logs the +// reason. Successful requests pass through unmodified. +func (v *Validator) Middleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if v.mode == ModeDisabled { + log.Printf("scheduledauth: WARN — request to %s allowed without auth (mode=disabled)", r.URL.Path) + next.ServeHTTP(w, r) + return + } + + authz := r.Header.Get("Authorization") + if err := v.Validate(r.Context(), authz); err != nil { + log.Printf("scheduledauth: rejected %s %s: %v", r.Method, r.URL.Path, err) + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + next.ServeHTTP(w, r) + }) +} + +// Validate is the unit-testable core. It returns nil iff the +// Authorization header satisfies the configured mode's requirements. +func (v *Validator) Validate(ctx context.Context, authz string) error { + switch v.mode { + case ModeDisabled: + return nil + case ModeBearer: + return v.validateBearer(authz) + case ModeOIDC: + return v.validateOIDC(ctx, authz) + default: + // Should be impossible — New rejects unknown modes. + return fmt.Errorf("%w: misconfigured validator", ErrUnauthorized) + } +} + +func (v *Validator) validateBearer(authz string) error { + if authz == "" { + return fmt.Errorf("%w: missing Authorization header", ErrUnauthorized) + } + if subtle.ConstantTimeCompare([]byte(authz), v.bearer) != 1 { + return fmt.Errorf("%w: bearer secret mismatch", ErrUnauthorized) + } + return nil +} + +func (v *Validator) validateOIDC(ctx context.Context, authz string) error { + rawToken, err := extractBearerToken(authz) + if err != nil { + return err + } + + // Verify signature, issuer, and algorithm via go-oidc. SkipClientIDCheck + // and SkipExpiryCheck are enabled because we apply our own multi-aud + // and skew-tolerant expiry checks below. + idToken, err := v.verifier.Verify(ctx, rawToken) + if err != nil { + return fmt.Errorf("%w: %v", ErrUnauthorized, err) + } + + // Re-parse the payload to access iat / nbf — IDToken exposes Expiry + // and IssuedAt but go-oidc's expiry check has no skew tolerance, and + // nbf is not exposed as a typed field. + var c claims + if err := idToken.Claims(&c); err != nil { + return fmt.Errorf("%w: malformed claims: %v", ErrUnauthorized, err) + } + + if err := v.checkTimestamps(c); err != nil { + return err + } + + // Audience: at least one of the token's aud values must be in our + // allow-list. The token typically has exactly one audience but the + // spec permits multiple. + if !audienceMatches(idToken.Audience, v.audiences) { + return fmt.Errorf("%w: audience %v not in allowlist", ErrUnauthorized, idToken.Audience) + } + + // Subject pinning: required defence-in-depth — any GCP SA in the + // org could mint a token with our `aud` value, but only the + // scheduler SA has our `sub`. + if _, ok := v.subjects[idToken.Subject]; !ok { + return fmt.Errorf("%w: subject %q not in allowlist", ErrUnauthorized, idToken.Subject) + } + + log.Printf("scheduledauth: oidc token accepted (sub=%s, aud=%v)", idToken.Subject, idToken.Audience) + return nil +} + +// extractBearerToken pulls the JWT out of an `Authorization: Bearer ` +// header. Returns ErrUnauthorized for any shape that isn't a non-empty +// bearer token. +func extractBearerToken(authz string) (string, error) { + if authz == "" { + return "", fmt.Errorf("%w: missing Authorization header", ErrUnauthorized) + } + const prefix = "Bearer " + if !strings.HasPrefix(authz, prefix) { + return "", fmt.Errorf("%w: Authorization is not a Bearer token", ErrUnauthorized) + } + raw := strings.TrimSpace(authz[len(prefix):]) + if raw == "" { + return "", fmt.Errorf("%w: empty bearer token", ErrUnauthorized) + } + return raw, nil +} + +// checkTimestamps applies skew-tolerant exp/nbf/iat checks. exp is +// required (Cloud Scheduler always sets it); nbf and iat are optional +// per RFC 7519. The iat-in-future check defends against horizontally- +// spread clock-skew attacks. +func (v *Validator) checkTimestamps(c claims) error { + now := v.now() + + if c.ExpiresAt == 0 { + return fmt.Errorf("%w: missing exp claim", ErrUnauthorized) + } + exp := time.Unix(c.ExpiresAt, 0) + if now.After(exp.Add(v.skew)) { + return fmt.Errorf("%w: token expired at %s (now=%s, skew=%s)", + ErrUnauthorized, exp.Format(time.RFC3339), now.Format(time.RFC3339), v.skew) + } + + if c.NotBefore != 0 { + nbf := time.Unix(c.NotBefore, 0) + if now.Add(v.skew).Before(nbf) { + return fmt.Errorf("%w: token not yet valid (nbf=%s, now=%s)", + ErrUnauthorized, nbf.Format(time.RFC3339), now.Format(time.RFC3339)) + } + } + + if c.IssuedAt != 0 { + iat := time.Unix(c.IssuedAt, 0) + if iat.After(now.Add(v.skew)) { + return fmt.Errorf("%w: token iat in the future (iat=%s, now=%s)", + ErrUnauthorized, iat.Format(time.RFC3339), now.Format(time.RFC3339)) + } + } + return nil +} + +func audienceMatches(tokenAud []string, allowed map[string]struct{}) bool { + for _, a := range tokenAud { + if _, ok := allowed[a]; ok { + return true + } + } + return false +} + +// IsUnauthorized reports whether err originated from a Validate failure. +// Useful for callers that need to distinguish auth failures from config +// errors. +func IsUnauthorized(err error) bool { + return errors.Is(err, ErrUnauthorized) +} diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go new file mode 100644 index 000000000..4af205581 --- /dev/null +++ b/internal/server/scheduledauth/validator_test.go @@ -0,0 +1,717 @@ +package scheduledauth + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + jose "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4/jwt" +) + +// testKey wraps an RSA keypair plus the kid we'll publish in the JWKS. +type testKey struct { + priv *rsa.PrivateKey + pub *rsa.PublicKey + kid string +} + +func newTestKey(t *testing.T, kid string) *testKey { + t.Helper() + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa keygen: %v", err) + } + return &testKey{priv: priv, pub: &priv.PublicKey, kid: kid} +} + +// jwks renders a JWKS document containing the given public keys. +func jwks(keys ...*testKey) []byte { + type jwk struct { + Kty string `json:"kty"` + Use string `json:"use"` + Kid string `json:"kid"` + Alg string `json:"alg"` + N string `json:"n"` + E string `json:"e"` + } + out := struct { + Keys []jwk `json:"keys"` + }{} + for _, k := range keys { + j := jose.JSONWebKey{Key: k.pub, KeyID: k.kid, Algorithm: string(jose.RS256), Use: "sig"} + raw, _ := j.MarshalJSON() + var parsed jwk + _ = json.Unmarshal(raw, &parsed) + out.Keys = append(out.Keys, parsed) + } + b, _ := json.Marshal(out) + return b +} + +// jwksServer returns an httptest.Server that serves a JWKS document at +// "/" and counts how many times it has been hit (atomic, so concurrent +// fetches under the single-flight test see a stable value). +type jwksServer struct { + *httptest.Server + hits *atomic.Int64 +} + +func newJWKSServer(t *testing.T, body []byte) *jwksServer { + t.Helper() + hits := &atomic.Int64{} + mu := &sync.Mutex{} + current := body + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + mu.Lock() + defer mu.Unlock() + w.Header().Set("Content-Type", "application/jwk-set+json") + _, _ = w.Write(current) + }) + mux.HandleFunc("/swap", func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + // Body of POST /swap replaces the served JWKS. Used to test + // refresh-on-unknown-kid. + buf := make([]byte, r.ContentLength) + _, _ = r.Body.Read(buf) + current = buf + w.WriteHeader(http.StatusOK) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return &jwksServer{Server: srv, hits: hits} +} + +// signToken signs a JWT with the given key and claim set. +func signToken(t *testing.T, key *testKey, c map[string]interface{}) string { + t.Helper() + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.RS256, Key: key.priv}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", key.kid), + ) + if err != nil { + t.Fatalf("signer: %v", err) + } + tok, err := jwt.Signed(signer).Claims(c).Serialize() + if err != nil { + t.Fatalf("sign: %v", err) + } + return tok +} + +// signTokenAlg signs with a non-RS256 algorithm. Used to confirm the +// validator rejects unexpected algs. +func signTokenAlg(t *testing.T, alg jose.SignatureAlgorithm, key interface{}, kid string, c map[string]interface{}) string { + t.Helper() + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: alg, Key: key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", kid), + ) + if err != nil { + t.Fatalf("signer: %v", err) + } + tok, err := jwt.Signed(signer).Claims(c).Serialize() + if err != nil { + t.Fatalf("sign: %v", err) + } + return tok +} + +func baseClaims(now time.Time, sub, aud, iss string) map[string]interface{} { + return map[string]interface{}{ + "iss": iss, + "sub": sub, + "aud": aud, + "exp": now.Add(5 * time.Minute).Unix(), + "iat": now.Add(-1 * time.Minute).Unix(), + } +} + +func newOIDCValidator(t *testing.T, jwksURL string) *Validator { + t.Helper() + v, err := New(Config{ + Mode: ModeOIDC, + Issuer: "https://accounts.example.com", + JWKSURL: jwksURL, + Audiences: []string{"https://api.example.com"}, + Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + return v +} + +func TestValidate_OIDC_Valid(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + if err := v.Validate(context.Background(), "Bearer "+tok); err != nil { + t.Fatalf("expected valid, got: %v", err) + } +} + +func TestValidate_OIDC_MissingHeader(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + if err := v.Validate(context.Background(), ""); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_NotBearer(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + if err := v.Validate(context.Background(), "Basic Zm9vOmJhcg=="); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_Expired(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + now := time.Now() + claims := baseClaims(now, + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com") + // 2 minutes past expiry, well beyond the 60s skew. + claims["exp"] = now.Add(-2 * time.Minute).Unix() + tok := signToken(t, key, claims) + + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_ExpiryWithinSkew(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + now := time.Now() + claims := baseClaims(now, + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com") + // Just expired but within the 60s skew window. + claims["exp"] = now.Add(-30 * time.Second).Unix() + tok := signToken(t, key, claims) + + // Pin time so the test is deterministic. + v.now = func() time.Time { return now } + if err := v.Validate(context.Background(), "Bearer "+tok); err != nil { + t.Fatalf("expected accepted within skew, got: %v", err) + } +} + +func TestValidate_OIDC_WrongAudience(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://attacker.example.com", + "https://accounts.example.com")) + + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_WrongIssuer(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://attacker-iss.com")) + + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_WrongSubject(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "attacker@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_BadSignature(t *testing.T) { + key := newTestKey(t, "kid-1") + other := newTestKey(t, "kid-1") // same kid, different key — sig won't verify + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, other, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_AlgorithmConfusion(t *testing.T) { + // Server publishes only an RS256 key. Client tries to sign an HS256 + // token using the public key as a shared secret (the classic alg + // confusion attack). The verifier MUST reject this because we + // pinned SupportedSigningAlgs to ["RS256"]. + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + // We can't easily sign an HS256 with an RSA public key via go-jose + // (it expects []byte), but signing with HS256 + a random secret + + // kid="kid-1" is a strictly easier case for the attacker; if the + // validator rejects this it covers the core alg-confusion gap. + tok := signTokenAlg(t, jose.HS256, []byte("attacker-secret-32-bytes-padding!!"), key.kid, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + err := v.Validate(context.Background(), "Bearer "+tok) + if !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized for HS256 token, got: %v", err) + } +} + +func TestValidate_OIDC_IATInFuture(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + now := time.Now() + claims := baseClaims(now, + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com") + // 2 minutes in the future, well beyond the 60s skew. + claims["iat"] = now.Add(2 * time.Minute).Unix() + tok := signToken(t, key, claims) + + v.now = func() time.Time { return now } + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_NotBeforeFuture(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + now := time.Now() + claims := baseClaims(now, + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com") + claims["nbf"] = now.Add(2 * time.Minute).Unix() + tok := signToken(t, key, claims) + + v.now = func() time.Time { return now } + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_OIDC_AudienceListClaim(t *testing.T) { + // `aud` may be either a string or a JSON list. Make sure the list + // form is accepted as long as one entry matches. + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + now := time.Now() + claims := baseClaims(now, + "scheduler@example.iam.gserviceaccount.com", + "placeholder", // overridden below + "https://accounts.example.com") + claims["aud"] = []string{"https://other.example.com", "https://api.example.com"} + tok := signToken(t, key, claims) + + if err := v.Validate(context.Background(), "Bearer "+tok); err != nil { + t.Fatalf("expected valid, got: %v", err) + } +} + +func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { + // 1. Server starts with key A. Token signed by A → validates. + // 2. Provider rotates: server now publishes key B. Token signed by B + // arrives with a previously-unseen kid. The validator MUST refresh + // the JWKS and accept the new token. + keyA := newTestKey(t, "kid-A") + keyB := newTestKey(t, "kid-B") + srv := newJWKSServer(t, jwks(keyA)) + v := newOIDCValidator(t, srv.URL) + + tokA := signToken(t, keyA, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + if err := v.Validate(context.Background(), "Bearer "+tokA); err != nil { + t.Fatalf("kid A: %v", err) + } + + // Swap the JWKS to publish kid B. + swap, _ := http.NewRequest("POST", srv.URL+"/swap", strings.NewReader(string(jwks(keyB)))) + swap.ContentLength = int64(len(jwks(keyB))) + resp, err := http.DefaultClient.Do(swap) + if err != nil { + t.Fatalf("swap: %v", err) + } + resp.Body.Close() + + tokB := signToken(t, keyB, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + if err := v.Validate(context.Background(), "Bearer "+tokB); err != nil { + t.Fatalf("kid B (post-rotation): %v", err) + } +} + +func TestValidate_OIDC_SingleFlight_StampedeProtection(t *testing.T) { + // 50 concurrent verifications of fresh tokens after a cold start + // should result in at most a small handful of JWKS fetches (the + // underlying RemoteKeySet uses single-flight). Without the + // protection we'd see ~50 hits. + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + const N = 50 + var wg sync.WaitGroup + wg.Add(N) + errCh := make(chan error, N) + for i := 0; i < N; i++ { + go func() { + defer wg.Done() + if err := v.Validate(context.Background(), "Bearer "+tok); err != nil { + errCh <- err + } + }() + } + wg.Wait() + close(errCh) + for err := range errCh { + t.Errorf("concurrent validate: %v", err) + } + hits := srv.hits.Load() + if hits > 5 { + t.Errorf("expected <=5 JWKS fetches under single-flight, got %d", hits) + } +} + +func TestValidate_Bearer_OK(t *testing.T) { + v, err := New(Config{Mode: ModeBearer, Bearer: "topsecret"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := v.Validate(context.Background(), "Bearer topsecret"); err != nil { + t.Fatalf("expected ok, got: %v", err) + } +} + +func TestValidate_Bearer_Mismatch(t *testing.T) { + v, err := New(Config{Mode: ModeBearer, Bearer: "topsecret"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := v.Validate(context.Background(), "Bearer wrong"); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_Bearer_MissingHeader(t *testing.T) { + v, err := New(Config{Mode: ModeBearer, Bearer: "topsecret"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := v.Validate(context.Background(), ""); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got: %v", err) + } +} + +func TestValidate_Disabled_AlwaysAllows(t *testing.T) { + v, err := New(Config{Mode: ModeDisabled}) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := v.Validate(context.Background(), ""); err != nil { + t.Fatalf("expected nil, got: %v", err) + } +} + +func TestNew_Rejects_OIDCWithoutSubjects(t *testing.T) { + _, err := New(Config{ + Mode: ModeOIDC, + Audiences: []string{"https://api.example.com"}, + // Subjects intentionally empty. + }) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } +} + +func TestNew_Rejects_OIDCWithoutAudiences(t *testing.T) { + _, err := New(Config{ + Mode: ModeOIDC, + Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + }) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } +} + +func TestNew_Rejects_BearerWithoutSecret(t *testing.T) { + _, err := New(Config{Mode: ModeBearer}) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } +} + +func TestNew_Rejects_UnknownMode(t *testing.T) { + _, err := New(Config{Mode: Mode("bogus")}) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } +} + +func TestLoadConfig_DefaultsToDisabled(t *testing.T) { + cfg, err := LoadConfig(EnvMap{}) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if cfg.Mode != ModeDisabled { + t.Fatalf("default mode = %s, want %s", cfg.Mode, ModeDisabled) + } +} + +func TestLoadConfig_OIDCParsesCommaSeparated(t *testing.T) { + cfg, err := LoadConfig(EnvMap{ + EnvAuthMode: "oidc", + EnvOIDCAudience: " a@example.com , , b@example.com ", + EnvOIDCSubjects: "x@example.com,y@example.com", + }) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if cfg.Mode != ModeOIDC { + t.Fatalf("mode = %s", cfg.Mode) + } + if got, want := strings.Join(cfg.Audiences, ","), "a@example.com,b@example.com"; got != want { + t.Fatalf("audiences = %q, want %q", got, want) + } + if got, want := strings.Join(cfg.Subjects, ","), "x@example.com,y@example.com"; got != want { + t.Fatalf("subjects = %q, want %q", got, want) + } +} + +func TestLoadConfig_RejectsUnknownMode(t *testing.T) { + _, err := LoadConfig(EnvMap{EnvAuthMode: "bogus"}) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } +} + +func TestLoadConfig_BearerParse(t *testing.T) { + cfg, err := LoadConfig(EnvMap{ + EnvAuthMode: "bearer", + EnvBearerSecret: "topsecret", + }) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if cfg.Mode != ModeBearer || cfg.Bearer != "topsecret" { + t.Fatalf("got %+v", cfg) + } +} + +func TestMiddleware_OIDC_RejectsAndLogsButDoesNotCallNext(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + called := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { called = true }) + mw := v.Middleware(next) + + req := httptest.NewRequest("POST", "/api/scheduled/foo", nil) + rr := httptest.NewRecorder() + mw.ServeHTTP(rr, req) + + if rr.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401", rr.Code) + } + if called { + t.Fatalf("next handler must not be called on auth failure") + } +} + +func TestMiddleware_OIDC_AllowsAndCallsNextOnSuccess(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + tok := signToken(t, key, baseClaims(time.Now(), + "scheduler@example.iam.gserviceaccount.com", + "https://api.example.com", + "https://accounts.example.com")) + + called := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + }) + mw := v.Middleware(next) + + req := httptest.NewRequest("POST", "/api/scheduled/foo", nil) + req.Header.Set("Authorization", "Bearer "+tok) + rr := httptest.NewRecorder() + mw.ServeHTTP(rr, req) + + if !called { + t.Fatalf("next handler should be called on success") + } + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rr.Code) + } +} + +func TestMiddleware_Disabled_PassesThroughWithWarn(t *testing.T) { + v, err := New(Config{Mode: ModeDisabled}) + if err != nil { + t.Fatalf("New: %v", err) + } + called := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusNoContent) + }) + mw := v.Middleware(next) + + req := httptest.NewRequest("POST", "/api/scheduled/foo", nil) + rr := httptest.NewRecorder() + mw.ServeHTTP(rr, req) + + if !called { + t.Fatalf("disabled middleware must call next") + } + if rr.Code != http.StatusNoContent { + t.Fatalf("status = %d", rr.Code) + } +} + +func TestWarmup_HitsJWKSEndpoint(t *testing.T) { + key := newTestKey(t, "kid-1") + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + before := srv.hits.Load() + v.Warmup(context.Background()) + after := srv.hits.Load() + if after <= before { + t.Fatalf("Warmup should have hit JWKS endpoint at least once (before=%d after=%d)", before, after) + } +} + +func TestWarmup_LoggedAndNonFatal_OnDeadEndpoint(t *testing.T) { + v, err := New(Config{ + Mode: ModeOIDC, + Issuer: "https://accounts.example.com", + JWKSURL: "http://127.0.0.1:1/this-port-is-closed", // ECONNREFUSED + Audiences: []string{"https://api.example.com"}, + Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + // Warmup must not panic or block. Use a short timeout to make the + // test fast even if Warmup were to misbehave. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + v.Warmup(ctx) // non-fatal — only logs +} + +// guard: sanity-check that Mode is reported back. +func TestMode(t *testing.T) { + for _, m := range []Mode{ModeOIDC, ModeBearer, ModeDisabled} { + var v *Validator + var err error + switch m { + case ModeOIDC: + v, err = New(Config{ + Mode: ModeOIDC, + JWKSURL: "http://127.0.0.1:1", + Audiences: []string{"a"}, + Subjects: []string{"s"}, + }) + case ModeBearer: + v, err = New(Config{Mode: ModeBearer, Bearer: "x"}) + case ModeDisabled: + v, err = New(Config{Mode: ModeDisabled}) + } + if err != nil { + t.Fatalf("New(%s): %v", m, err) + } + if v.Mode() != m { + t.Fatalf("Mode() = %s, want %s", v.Mode(), m) + } + } +} + +// guard: ensure typed sentinel works with errors.Is. +func TestIsUnauthorized(t *testing.T) { + wrapped := fmt.Errorf("wrap: %w", ErrUnauthorized) + if !IsUnauthorized(wrapped) { + t.Fatalf("IsUnauthorized failed for wrapped sentinel") + } + if IsUnauthorized(errors.New("other")) { + t.Fatalf("IsUnauthorized matched unrelated error") + } +} From 59efced10e927ef795569db21d8662812858893e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 20:46:23 +0200 Subject: [PATCH 03/15] fix(server): replace bearer-only check with OIDC-aware middleware on /api/scheduled/* (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the new `internal/server/scheduledauth` validator into the HTTP server. Replaces the inline `subtle.ConstantTimeCompare` against `appConfig.ScheduledTaskSecret` in `handleScheduledHTTP` with a proper middleware applied at mux registration time, so: - Auth is enforced before any handler logic runs (including the method check — invalid methods now return 401 in authenticated mode rather than disclosing which methods exist). - The validator picks the right auth scheme per platform: oidc on GCP, bearer on Azure, disabled for local dev (with loud WARN logs). - The bearer secret in bearer mode comes from `cfg.ScheduledTaskSecret`, which `resolveScheduledTaskSecret` already populates from Key Vault / Secrets Manager via the SecretResolver, so the plaintext never lives in a container env var. Validator construction is fail-fast: misconfig (empty subjects in oidc mode, unknown mode, etc.) returns an error from `NewApplicationFromDeps` and the container refuses to start. A best-effort JWKS warmup runs at startup to surface misconfiguration early without blocking on Google's CDN. Existing `TestHandleScheduledHTTP` cases that exercised the inline bearer check are migrated to drive through the middleware via the new `scheduledAuthMiddleware` helper, mirroring how `CreateHTTPServer` wires it. The remaining cases (method check, path validation) keep `app.scheduledAuth = nil` — the helper passes through unmodified, so those tests continue to validate the handler in isolation. --- internal/server/app.go | 53 ++++++++++++++++++++++++++++++++++++ internal/server/http.go | 36 ++++++++++++++---------- internal/server/http_test.go | 24 ++++++++++++++-- 3 files changed, 96 insertions(+), 17 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index 14a2dca32..ec341b964 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -27,6 +27,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/runtime" "github.com/LeanerCloud/CUDly/internal/scheduler" "github.com/LeanerCloud/CUDly/internal/secrets" + "github.com/LeanerCloud/CUDly/internal/server/scheduledauth" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -77,6 +78,14 @@ type Application struct { // federated credential path). Nil when the deployment has not // opted into the federated flow. signer oidc.Signer + + // scheduledAuth authenticates inbound /api/scheduled/* requests. + // Always non-nil — disabled mode allows everything through with a + // loud WARN log. In oidc mode (GCP) it validates the Cloud + // Scheduler-signed ID token; in bearer mode (Azure) it constant- + // time-compares the shared secret resolved at startup from Key + // Vault. + scheduledAuth *scheduledauth.Validator } // ApplicationConfig holds all env-based configuration for the application @@ -267,6 +276,35 @@ func resolveScheduledTaskSecret(ctx context.Context, cfg ApplicationConfig, reso return resolved } +// envSourceOS implements scheduledauth.EnvSource against os.Getenv. The +// scheduledauth package owns the env var names and parse rules so they +// stay aligned with Terraform. +type envSourceOS struct{} + +func (envSourceOS) Get(key string) string { return os.Getenv(key) } + +// buildScheduledAuth wires up the /api/scheduled/* validator. It pulls +// mode + OIDC params from env (they're per-deployment Terraform inputs) +// but injects the bearer secret from cfg — that path goes through the +// SecretResolver (Key Vault) for production deployments and never lives +// in a container env var. Returns ErrConfigInvalid on misconfig so the +// container fails fast rather than silently accepting everything. +func buildScheduledAuth(cfg ApplicationConfig) (*scheduledauth.Validator, error) { + saCfg, err := scheduledauth.LoadConfig(envSourceOS{}) + if err != nil { + return nil, err + } + // In bearer mode, override the env-supplied secret with the one + // already resolved from KV / SM. LoadConfig reads SCHEDULED_TASK_SECRET + // directly which is fine for local dev where the env carries the + // plaintext, but in prod cfg.ScheduledTaskSecret is the authoritative + // value (resolved by resolveScheduledTaskSecret upstream). + if saCfg.Mode == scheduledauth.ModeBearer { + saCfg.Bearer = cfg.ScheduledTaskSecret + } + return scheduledauth.New(saCfg) +} + // NewApplicationFromDeps creates an Application from pre-built configuration and dependencies. // This is the testable constructor - all external I/O is done before calling this. func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps ExternalDeps) (*Application, error) { @@ -276,6 +314,20 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext cfg.ScheduledTaskSecret = resolveScheduledTaskSecret(ctx, cfg, deps.SecretResolver) + // Build the /api/scheduled/* auth validator. Fail-fast on bad + // config (empty subjects in oidc mode, unknown mode, etc.) — better + // to crash on startup than to silently accept unauthenticated + // scheduled-task calls in production. + scheduledAuth, err := buildScheduledAuth(cfg) + if err != nil { + return nil, fmt.Errorf("scheduled-task auth init: %w", err) + } + // Best-effort JWKS warmup — surfaces misconfiguration in startup + // logs without blocking startup if Google's CDN is unreachable. + warmCtx, warmCancel := context.WithTimeout(ctx, 5*time.Second) + scheduledAuth.Warmup(warmCtx) + warmCancel() + // Construct the OIDC issuer signer once per deployment. Nil means // the deployment has not opted into the federated flow yet — all // OIDC-dependent paths (handler_oidc.go, purchase manager Azure @@ -381,6 +433,7 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext secretResolver: deps.SecretResolver, appConfig: cfg, signer: signer, + scheduledAuth: scheduledAuth, }, nil } diff --git a/internal/server/http.go b/internal/server/http.go index 6046401a6..9bf6bc232 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -2,7 +2,6 @@ package server import ( "context" - "crypto/subtle" "encoding/base64" "encoding/json" "fmt" @@ -20,9 +19,13 @@ import ( func CreateHTTPServer(app *Application, port int) *http.Server { mux := http.NewServeMux() - // Register health and scheduled task routes (always available) + // Register health and scheduled task routes (always available). + // /api/scheduled/* sits behind the scheduledauth middleware — see + // internal/server/scheduledauth. Mode is selected by + // SCHEDULED_TASK_AUTH_MODE: oidc on GCP, bearer on Azure, disabled + // for local dev. mux.HandleFunc("/health", app.handleHealthCheck) - mux.HandleFunc("/api/scheduled/", app.handleScheduledHTTP) + mux.Handle("/api/scheduled/", app.scheduledAuthMiddleware(http.HandlerFunc(app.handleScheduledHTTP))) // When STATIC_DIR is set, serve static files for non-API paths // and route only /api/ to the API handler. @@ -105,24 +108,29 @@ func (app *Application) handleHTTPRequest(w http.ResponseWriter, r *http.Request lambdaResponseToHTTP(w, lambdaResp) } +// scheduledAuthMiddleware returns the configured scheduledauth middleware, +// or a passthrough no-op if app.scheduledAuth is nil. Tests that build +// an Application directly (without NewApplicationFromDeps) leave it +// nil — they exercise the handler in isolation, with the middleware +// covered separately in scheduledauth's own tests. +func (app *Application) scheduledAuthMiddleware(next http.Handler) http.Handler { + if app.scheduledAuth == nil { + return next + } + return app.scheduledAuth.Middleware(next) +} + // handleScheduledHTTP handles scheduled tasks via HTTP endpoint -// This is used by GCP Cloud Scheduler and Azure Logic Apps +// This is used by GCP Cloud Scheduler and Azure Logic Apps. Auth is +// enforced upstream by scheduledAuthMiddleware (see CreateHTTPServer); +// by the time we reach here the request is already authenticated for +// the configured mode. func (app *Application) handleScheduledHTTP(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) return } - // Verify shared secret for scheduled task authentication - if secret := app.appConfig.ScheduledTaskSecret; secret != "" { - provided := r.Header.Get("Authorization") - expected := "Bearer " + secret - if subtle.ConstantTimeCompare([]byte(provided), []byte(expected)) != 1 { - http.Error(w, "Unauthorized", http.StatusUnauthorized) - return - } - } - ctx := r.Context() // Ensure database connection is established (lazy initialization) diff --git a/internal/server/http_test.go b/internal/server/http_test.go index 3df54ba5a..128be90b8 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -12,6 +12,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/api" "github.com/LeanerCloud/CUDly/internal/scheduler" + "github.com/LeanerCloud/CUDly/internal/server/scheduledauth" "github.com/LeanerCloud/CUDly/internal/testutil" "github.com/aws/aws-lambda-go/events" ) @@ -263,7 +264,14 @@ func TestHandleScheduledHTTP(t *testing.T) { method: "POST", path: "/api/scheduled/collect_recommendations", setupApp: func(app *Application) { - app.appConfig.ScheduledTaskSecret = "my-secret" + v, err := scheduledauth.New(scheduledauth.Config{ + Mode: scheduledauth.ModeBearer, + Bearer: "my-secret", + }) + if err != nil { + panic(err) + } + app.scheduledAuth = v }, expectedStatus: 401, }, @@ -273,7 +281,14 @@ func TestHandleScheduledHTTP(t *testing.T) { path: "/api/scheduled/collect_recommendations", authHeader: "Bearer my-secret", setupApp: func(app *Application) { - app.appConfig.ScheduledTaskSecret = "my-secret" + v, err := scheduledauth.New(scheduledauth.Config{ + Mode: scheduledauth.ModeBearer, + Bearer: "my-secret", + }) + if err != nil { + panic(err) + } + app.scheduledAuth = v app.Scheduler = &testutil.MockScheduler{ CollectRecommendationsFunc: func(ctx context.Context) (*scheduler.CollectResult, error) { return &scheduler.CollectResult{}, nil @@ -297,7 +312,10 @@ func TestHandleScheduledHTTP(t *testing.T) { } w := httptest.NewRecorder() - app.handleScheduledHTTP(w, req) + // Drive the request through the same middleware chain + // that CreateHTTPServer wires up, so auth is enforced + // upstream of handleScheduledHTTP. + app.scheduledAuthMiddleware(http.HandlerFunc(app.handleScheduledHTTP)).ServeHTTP(w, req) testutil.AssertEqual(t, tt.expectedStatus, w.Code) From 8ef51a59bb4cb09e1bdcd743ede9f7cc4848af23 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 20:54:27 +0200 Subject: [PATCH 04/15] fix(terraform/gcp): pin oidc_token.audience + wire scheduled-task auth env vars (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the auth gap on GCP that #161 surfaced: removing the plaintext `Authorization: Bearer ${var.scheduled_task_secret}` header from Cloud Scheduler left `/api/scheduled/*` unauthenticated on Cloud Run and GKE today (both have `cloud_run_allow_unauthenticated = true` and GKE has no IAP / IAM gate). This commit completes Option B: the in-app OIDC validator (added in earlier commits) now has explicit, deterministic config for the audience and subject claims it must accept. Cloud Run + GKE modules: - Set `oidc_token.audience` explicitly to the scheduler SA email on every `google_cloud_scheduler_job.http_target` block. Without this, Cloud Scheduler defaults `audience` to the target URL — coupling the validator's allow-list to the Cloud Run URL shape and breaking on Function-URL / vanity-domain changes. The explicit value mirrors the `email` we add to `SCHEDULED_TASK_OIDC_AUDIENCE` env. - Wire `SCHEDULED_TASK_AUTH_MODE`, `SCHEDULED_TASK_OIDC_AUDIENCE`, `SCHEDULED_TASK_OIDC_SUBJECTS` env vars on the running container. `SUBJECTS` is pinned to the SA's `unique_id` (the value Google sets in the JWT `sub` claim for SA-signed ID tokens), `AUDIENCE` to the SA email. When neither scheduler is enabled, the mode falls back to `disabled` — matches the old behaviour of skipping auth when no secret was set. Azure container-apps + AKS modules: - Set `SCHEDULED_TASK_AUTH_MODE = "bearer"`. The existing `SCHEDULED_TASK_SECRET_NAME` (Key Vault secret name, NOT value) flow is unchanged — `internal/server/app.go::resolveScheduledTaskSecret` resolves it via the SecretResolver at startup and the validator constant-time-compares it. Logic Apps' HTTP Connector trigger does not emit Entra OIDC tokens, so OIDC mode is not viable on Azure. tfvars examples (gcp/dev, azure/dev): document that the `SCHEDULED_*` env vars are wired automatically by the modules — users do not set them in `*.tfvars`. AWS Lambda is unaffected (EventBridge runs ECS tasks directly via `RunTask`; the HTTP `/api/scheduled/*` route is not on the critical path on AWS). #78 (flipping `cloud_run_allow_unauthenticated = false`) remains worth doing as defence-in-depth but is independent of this change — Option B works whether or not Cloud Run's IAM gate is enforced. --- .../environments/azure/dev.tfvars.example | 7 +++ terraform/environments/gcp/dev.tfvars.example | 6 ++ terraform/modules/compute/azure/aks/main.tf | 9 +++ .../compute/azure/container-apps/main.tf | 7 +++ .../modules/compute/gcp/cloud-run/main.tf | 57 +++++++++++++++++-- terraform/modules/compute/gcp/gke/main.tf | 44 ++++++++++++-- 6 files changed, 120 insertions(+), 10 deletions(-) diff --git a/terraform/environments/azure/dev.tfvars.example b/terraform/environments/azure/dev.tfvars.example index 318bcdc85..cc0f1ffce 100644 --- a/terraform/environments/azure/dev.tfvars.example +++ b/terraform/environments/azure/dev.tfvars.example @@ -78,6 +78,13 @@ key_vault_default_network_acl_action = "Allow" # ============================================== # Scheduled Tasks (Logic Apps) # ============================================== +# +# Logic Apps fetch a shared bearer secret from Key Vault at invocation +# time and call /api/scheduled/* with `Authorization: Bearer `. +# The CUDly app validates via SCHEDULED_TASK_AUTH_MODE=bearer (set +# automatically by the container-apps / aks modules) — the secret name +# itself is wired through `module.secrets.scheduled_task_secret_name` +# and never lives in the container env. enable_scheduled_tasks = true recommendation_schedule = "0 2 * * *" # Cron: Daily at 2 AM diff --git a/terraform/environments/gcp/dev.tfvars.example b/terraform/environments/gcp/dev.tfvars.example index 8a6619982..508a0b89c 100644 --- a/terraform/environments/gcp/dev.tfvars.example +++ b/terraform/environments/gcp/dev.tfvars.example @@ -65,6 +65,12 @@ enable_nat_logging = false # ============================================== # Scheduled Tasks (Cloud Scheduler) # ============================================== +# +# Cloud Scheduler signs OIDC ID tokens with `audience` and `sub` set +# from the per-job service account. The CUDly app validates these +# tokens via SCHEDULED_TASK_AUTH_MODE=oidc + SCHEDULED_TASK_OIDC_* +# env vars — those are wired automatically by the cloud-run / gke +# modules and do not need to be set here. enable_scheduled_tasks = true recommendation_schedule = "0 2 * * *" # Cron: Daily at 2 AM diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index cecae74cc..f9fe58ccc 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -360,6 +360,15 @@ resource "kubernetes_deployment" "app" { value = join(",", var.allowed_origins) } + # Scheduled-task auth — Azure stays on bearer mode (Logic Apps + # fetch the shared secret from Key Vault at invocation time; + # Logic Apps' HTTP Connector does not emit Entra OIDC tokens). + # Validated app-side via internal/server/scheduledauth. + env { + name = "SCHEDULED_TASK_AUTH_MODE" + value = "bearer" + } + dynamic "env" { for_each = var.additional_env_vars content { diff --git a/terraform/modules/compute/azure/container-apps/main.tf b/terraform/modules/compute/azure/container-apps/main.tf index 1a18fb0f7..4c367d5f9 100644 --- a/terraform/modules/compute/azure/container-apps/main.tf +++ b/terraform/modules/compute/azure/container-apps/main.tf @@ -137,6 +137,13 @@ resource "azurerm_container_app" "main" { CUDLY_SOURCE_CLOUD = "azure" CUDLY_SIGNING_KEY_VAULT_URL = var.key_vault_uri CUDLY_SIGNING_KEY_NAME = var.signing_key_name + # Scheduled-task auth — Azure stays on the bearer mode where + # Logic Apps fetch the shared secret from Key Vault at + # invocation time and the app constant-time-compares it via + # internal/server/scheduledauth. Switching to oidc here + # would require Entra-issued tokens, which Logic Apps does + # not emit on the HTTP Connector trigger we use. + SCHEDULED_TASK_AUTH_MODE = "bearer" }, var.additional_env_vars ) diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 5685a7eb1..74ea5ce74 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -34,6 +34,36 @@ resource "google_service_account" "cloud_run" { project = var.project_id } +# ============================================== +# Scheduled-task auth wiring +# ============================================== +# +# Both Cloud Scheduler jobs below sign OIDC tokens with `audience` set to +# the scheduler SA email and `sub` set to the SA's unique numeric ID. +# The Go validator (internal/server/scheduledauth) checks signature + +# issuer + audience + subject; pinning these here keeps the allow-list +# in the container env in sync with what Cloud Scheduler actually emits. +# +# `compact` drops empty strings produced when a scheduler is disabled +# (count = 0 → google_service_account.scheduler[0] is unavailable, so +# we use try() to short-circuit). `join(",")` produces the CSV format +# the validator parses. +locals { + scheduled_task_oidc_audiences = join(",", compact([ + var.enable_scheduled_tasks ? try(google_service_account.scheduler[0].email, "") : "", + var.enable_ri_exchange_schedule ? try(google_service_account.scheduler_ri_exchange[0].email, "") : "", + ])) + scheduled_task_oidc_subjects = join(",", compact([ + var.enable_scheduled_tasks ? try(google_service_account.scheduler[0].unique_id, "") : "", + var.enable_ri_exchange_schedule ? try(google_service_account.scheduler_ri_exchange[0].unique_id, "") : "", + ])) + # Auth mode selector. When neither scheduler is enabled there's + # nothing to validate; staying on "disabled" avoids fail-fast on an + # empty allow-list in the validator. The resulting WARN log is + # harmless when /api/scheduled/* is never hit. + scheduled_task_auth_mode = (var.enable_scheduled_tasks || var.enable_ri_exchange_schedule) ? "oidc" : "disabled" +} + # ============================================== # Cloud Run Service # ============================================== @@ -99,6 +129,11 @@ resource "google_cloud_run_v2_service" "main" { # OIDC issuer signing key — see internal/oidc/gcp_signer.go. CUDLY_SOURCE_CLOUD = "gcp" CUDLY_SIGNING_KEY_RESOURCE = local.signing_key_version_resource + # Scheduled-task OIDC validator config — see + # internal/server/scheduledauth and the locals block above. + SCHEDULED_TASK_AUTH_MODE = local.scheduled_task_auth_mode + SCHEDULED_TASK_OIDC_AUDIENCE = local.scheduled_task_oidc_audiences + SCHEDULED_TASK_OIDC_SUBJECTS = local.scheduled_task_oidc_subjects }, var.additional_env_vars ) @@ -270,14 +305,24 @@ resource "google_cloud_scheduler_job" "recommendations" { uri = "${google_cloud_run_v2_service.main.uri}/api/scheduled/recommendations" # Auth: oidc_token below is signed by the scheduler's service - # account at invocation time and validated by Cloud Run's IAM gate - # (roles/run.invoker grants this SA access; cloud_run_allow_unauthenticated - # defaults to false). The previous static `Authorization: Bearer + # account at invocation time. Two complementary defences: + # 1. Cloud Run's IAM gate via roles/run.invoker (when + # cloud_run_allow_unauthenticated = false; tracked separately + # in #78). + # 2. App-level OIDC validation on /api/scheduled/* — the Go + # validator (internal/server/scheduledauth) checks the JWT + # signature, issuer, audience, and pins the subject to this + # SA. Pinning `audience` here explicitly to the SA email + # makes the validator's allow-list deterministic; without an + # explicit value Cloud Scheduler defaults audience to the + # target URL, which would couple #159's fix to the Cloud Run + # URL shape. + # The previous static `Authorization: Bearer # ${var.scheduled_task_secret}` header leaked the shared secret into # the scheduler resource definition + Terraform state — closes #159. - # OIDC supersedes the application-level bearer check on GCP. oidc_token { service_account_email = google_service_account.scheduler[0].email + audience = google_service_account.scheduler[0].email } } } @@ -326,9 +371,11 @@ resource "google_cloud_scheduler_job" "ri_exchange" { uri = "${google_cloud_run_v2_service.main.uri}/api/scheduled/ri-exchange" # Same OIDC-only model as the recommendations scheduler above — - # see #159 for the rationale. + # see #159 for the rationale. Audience pinned to this SA email so + # the app-level validator's allow-list stays deterministic. oidc_token { service_account_email = google_service_account.scheduler_ri_exchange[0].email + audience = google_service_account.scheduler_ri_exchange[0].email } } } diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 8658b85ad..54d5bc688 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -18,6 +18,18 @@ locals { managed_by = "terraform" } ) + + # Scheduled-task OIDC validator wiring — see + # internal/server/scheduledauth. Audience pinned to the scheduler SA + # email (matches the explicit `audience` on the oidc_token block); + # subject pinned to the SA's unique numeric ID (the value Google puts + # in the JWT `sub` claim for SA-signed ID tokens). When the scheduler + # is disabled the env vars become empty strings and the validator + # stays in "disabled" mode (with a startup WARN log) — matches the + # cloud-run module's behaviour. + scheduled_task_oidc_audience = try(google_service_account.scheduler[0].email, "") + scheduled_task_oidc_subject = try(google_service_account.scheduler[0].unique_id, "") + scheduled_task_auth_mode = var.enable_scheduled_tasks ? "oidc" : "disabled" } # ============================================== @@ -448,6 +460,23 @@ resource "kubernetes_deployment" "app" { value = join(",", var.allowed_origins) } + # Scheduled-task OIDC validator config — see locals block above + # and internal/server/scheduledauth. + env { + name = "SCHEDULED_TASK_AUTH_MODE" + value = local.scheduled_task_auth_mode + } + + env { + name = "SCHEDULED_TASK_OIDC_AUDIENCE" + value = local.scheduled_task_oidc_audience + } + + env { + name = "SCHEDULED_TASK_OIDC_SUBJECTS" + value = local.scheduled_task_oidc_subject + } + dynamic "env" { for_each = var.additional_env_vars content { @@ -672,15 +701,20 @@ resource "google_cloud_scheduler_job" "recommendations" { http_method = "POST" uri = "${var.app_url}/api/scheduled/recommendations" - # Auth: oidc_token below is signed by the scheduler's service account - # at invocation time. The previous static `Authorization: Bearer + # Auth: oidc_token below is signed by the scheduler's service + # account at invocation time. GKE Ingress has no Cloud-Run-style + # IAM gate, so the OIDC token is validated by the app at + # /api/scheduled/* via internal/server/scheduledauth — signature, + # issuer, audience, and `sub` pinned to this SA. Audience is set + # explicitly here so the validator's allow-list stays + # deterministic; without it Cloud Scheduler defaults audience to + # the target URL. + # The previous static `Authorization: Bearer # ${var.scheduled_task_secret}` header leaked the shared secret into # the scheduler resource definition + Terraform state — closes #159. - # OIDC supersedes the application-level bearer check on GCP. (The - # GKE-side var was also defaulting to "" in practice — the static - # header was sending an empty Bearer header anyway.) oidc_token { service_account_email = google_service_account.scheduler[0].email + audience = google_service_account.scheduler[0].email } } } From db4925c6beac88781dee5f911a5475c70aab7a27 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 20:55:51 +0200 Subject: [PATCH 05/15] test(server): integration test for scheduled-task OIDC middleware (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `internal/server/scheduledauth/integration_test.go` exercising the full HTTP roundtrip: middleware in front of an inner handler, JWT signed by a test key, JWKS published over an httptest server, request sent through `http.DefaultClient` so net/http header parsing and the validator's lazy JWKS fetch are both on the path. Five sub-cases, each independently verifiable: 1. Valid token signed by published key → 200, handler runs. 2. Token signed by a key absent from the JWKS → 401, handler skipped. 3. Missing Authorization header → 401. 4. Wrong `sub` (subject pinning) → 401. 5. Wrong `aud` → 401. Reuses the helpers from `validator_test.go` (same package) to keep test infrastructure DRY. --- .../server/scheduledauth/integration_test.go | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 internal/server/scheduledauth/integration_test.go diff --git a/internal/server/scheduledauth/integration_test.go b/internal/server/scheduledauth/integration_test.go new file mode 100644 index 000000000..34eba032f --- /dev/null +++ b/internal/server/scheduledauth/integration_test.go @@ -0,0 +1,151 @@ +package scheduledauth + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// TestIntegration_FullHTTPRoundtrip drives the validator through an +// actual HTTP server with a mocked JWKS endpoint, exercising the same +// path the running container uses: middleware in front of an inner +// handler, JWKS fetched lazily, JWT verified end-to-end. +// +// The test is split into table-driven sub-cases so each scenario gets +// an independent verdict in `go test` output. It reuses the +// helpers (newTestKey, signToken, jwks, newJWKSServer, baseClaims) from +// validator_test.go since both files share the same test package. +func TestIntegration_FullHTTPRoundtrip(t *testing.T) { + // Cloud Scheduler uses one signing key per Google identity; rotate + // is handled by refresh-on-unknown-kid and covered in unit tests. + signingKey := newTestKey(t, "kid-prod-2026") + otherKey := newTestKey(t, "kid-attacker") + + // JWKS server publishes the prod key only. otherKey is not in the + // JWKS, so tokens signed by it must fail. + jwksSrv := newJWKSServer(t, jwks(signingKey)) + + v, err := New(Config{ + Mode: ModeOIDC, + Issuer: "https://accounts.example.com", + JWKSURL: jwksSrv.URL, + Audiences: []string{"https://cudly.example.com"}, + Subjects: []string{"scheduler@cudly.iam.gserviceaccount.com"}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + + // Inner handler the middleware fronts. Returns 200 + body so we can + // confirm the request reached the handler in the success case. + const handlerBody = "scheduled-task-ran" + handlerCalls := 0 + inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handlerCalls++ + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(handlerBody)) + }) + + // Wire validator middleware + inner handler behind a real HTTP + // server so we exercise net/http header parsing, query routing, + // and the actual JWKS fetch over the wire. + srv := httptest.NewServer(v.Middleware(inner)) + t.Cleanup(srv.Close) + + type tc struct { + name string + buildAuth func(t *testing.T) string + wantStatus int + wantHandlerHit bool + } + + now := time.Now() + cases := []tc{ + { + name: "valid token signed by published key", + buildAuth: func(t *testing.T) string { + return "Bearer " + signToken(t, signingKey, baseClaims(now, + "scheduler@cudly.iam.gserviceaccount.com", + "https://cudly.example.com", + "https://accounts.example.com")) + }, + wantStatus: http.StatusOK, + wantHandlerHit: true, + }, + { + name: "token signed by key not in JWKS", + buildAuth: func(t *testing.T) string { + return "Bearer " + signToken(t, otherKey, baseClaims(now, + "scheduler@cudly.iam.gserviceaccount.com", + "https://cudly.example.com", + "https://accounts.example.com")) + }, + wantStatus: http.StatusUnauthorized, + wantHandlerHit: false, + }, + { + name: "missing Authorization header", + buildAuth: func(t *testing.T) string { return "" }, + wantStatus: http.StatusUnauthorized, + wantHandlerHit: false, + }, + { + name: "wrong subject (audience matches but sub does not)", + buildAuth: func(t *testing.T) string { + return "Bearer " + signToken(t, signingKey, baseClaims(now, + "random@cudly.iam.gserviceaccount.com", + "https://cudly.example.com", + "https://accounts.example.com")) + }, + wantStatus: http.StatusUnauthorized, + wantHandlerHit: false, + }, + { + name: "wrong audience (sub matches but aud does not)", + buildAuth: func(t *testing.T) string { + return "Bearer " + signToken(t, signingKey, baseClaims(now, + "scheduler@cudly.iam.gserviceaccount.com", + "https://attacker.example.com", + "https://accounts.example.com")) + }, + wantStatus: http.StatusUnauthorized, + wantHandlerHit: false, + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + callsBefore := handlerCalls + + req, err := http.NewRequestWithContext(context.Background(), + http.MethodPost, srv.URL+"/api/scheduled/recommendations", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + if authz := tt.buildAuth(t); authz != "" { + req.Header.Set("Authorization", authz) + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("client Do: %v", err) + } + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + + if resp.StatusCode != tt.wantStatus { + t.Fatalf("status = %d (want %d), body=%q", resp.StatusCode, tt.wantStatus, string(body)) + } + gotHit := handlerCalls > callsBefore + if gotHit != tt.wantHandlerHit { + t.Fatalf("handler hit = %v, want %v", gotHit, tt.wantHandlerHit) + } + if tt.wantHandlerHit && string(body) != handlerBody { + t.Fatalf("body = %q, want %q", string(body), handlerBody) + } + }) + } +} From bb0f4411557571e478427a933803c074490016f9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 21:11:04 +0200 Subject: [PATCH 06/15] =?UTF-8?q?refactor(server):=20address=20CodeRabbit?= =?UTF-8?q?=20pass=201=20=E2=80=94=20JWKS=20warmup=20deadline,=20doc=20fix?= =?UTF-8?q?es,=20test=20panics=20(#161)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 of CodeRabbit feedback. Triage: Actionable — addressed: - internal/server/scheduledauth/validator_test.go: /swap test handler read body via single Body.Read which can return a short read and truncate the JWKS payload — switched to io.ReadAll, mutex held only while assigning the buffer. - terraform/environments/gcp/compute.tf: comment claimed "Application- level bearer auth is dropped on GCP" — that's stale now that we validate OIDC at the application level. Rewritten to describe the actual setup (OIDC at app + Cloud Run IAM gate as defence in depth) and to call out #78 separately. Nitpicks — addressed: - internal/server/scheduledauth/validator.go::Warmup: http.DefaultClient has no timeout, so a misconfigured JWKS URL could block startup indefinitely. Added a 5s fallback deadline when the caller's ctx has no deadline; respect any deadline the caller already set. - internal/server/scheduledauth/errors.go: package doc referenced a developer-local plan path. Replaced with a portable reference to PR #161 / issue #159. - internal/server/http_test.go: setupApp closures called panic(err) on scheduledauth.New failure. Switched the signature to take *testing.T and use testutil.AssertNoError, scoped via a small newBearerValidator helper to keep the table-driven cases tight. - internal/server/app.go: scheduledAuth field comment claimed "Always non-nil" which is true post-NewApplicationFromDeps but not for hand-built test Applications. Rewritten to spell out both paths. Actionable — dismissed with justification (will reply on PR thread): - "scheduledAuthMiddleware should fail-closed when nil": intentional test affordance; production path always populates the field via NewApplicationFromDeps. - "LoadConfig should reject empty SCHEDULED_TASK_AUTH_MODE": local-dev default-to-disabled+WARN is by design (plan §1). - "scheduled_task_auth_mode should default to oidc when both schedulers off": oidc mode requires non-empty audiences/subjects (validator fails fast otherwise), so disabled is the only correct value when no scheduler is configured. Same reasoning for cloud-run and gke. --- internal/server/app.go | 13 +++--- internal/server/http_test.go | 45 +++++++++---------- internal/server/scheduledauth/errors.go | 2 +- internal/server/scheduledauth/validator.go | 15 +++++++ .../server/scheduledauth/validator_test.go | 16 ++++--- terraform/environments/gcp/compute.tf | 13 +++--- 6 files changed, 65 insertions(+), 39 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index ec341b964..b132d3177 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -80,11 +80,14 @@ type Application struct { signer oidc.Signer // scheduledAuth authenticates inbound /api/scheduled/* requests. - // Always non-nil — disabled mode allows everything through with a - // loud WARN log. In oidc mode (GCP) it validates the Cloud - // Scheduler-signed ID token; in bearer mode (Azure) it constant- - // time-compares the shared secret resolved at startup from Key - // Vault. + // Non-nil after NewApplicationFromDeps — disabled mode allows + // everything through with a loud WARN log; oidc mode (GCP) + // validates the Cloud Scheduler-signed ID token; bearer mode + // (Azure) constant-time-compares the shared secret resolved at + // startup from Key Vault. May be nil in tests that build + // Application directly without the constructor; the + // scheduledAuthMiddleware helper passes through unmodified in + // that case so handler-only tests stay focused. scheduledAuth *scheduledauth.Validator } diff --git a/internal/server/http_test.go b/internal/server/http_test.go index 128be90b8..b13e5da9a 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -217,12 +217,25 @@ func TestLambdaResponseToHTTP(t *testing.T) { } func TestHandleScheduledHTTP(t *testing.T) { + // newBearerValidator constructs a bearer-mode validator and fails + // the test on construction error — preferred over `panic` because + // failures stay scoped to the offending case. + newBearerValidator := func(t *testing.T, secret string) *scheduledauth.Validator { + t.Helper() + v, err := scheduledauth.New(scheduledauth.Config{ + Mode: scheduledauth.ModeBearer, + Bearer: secret, + }) + testutil.AssertNoError(t, err) + return v + } + tests := []struct { name string method string path string authHeader string - setupApp func(*Application) + setupApp func(*testing.T, *Application) expectedStatus int expectError bool }{ @@ -230,7 +243,7 @@ func TestHandleScheduledHTTP(t *testing.T) { name: "valid scheduled task", method: "POST", path: "/api/scheduled/collect_recommendations", - setupApp: func(app *Application) { + setupApp: func(_ *testing.T, app *Application) { app.Scheduler = &testutil.MockScheduler{ CollectRecommendationsFunc: func(ctx context.Context) (*scheduler.CollectResult, error) { return &scheduler.CollectResult{ @@ -247,7 +260,7 @@ func TestHandleScheduledHTTP(t *testing.T) { name: "invalid method (GET instead of POST)", method: "GET", path: "/api/scheduled/collect_recommendations", - setupApp: func(app *Application) {}, + setupApp: func(_ *testing.T, _ *Application) {}, expectedStatus: 405, expectError: false, }, @@ -255,7 +268,7 @@ func TestHandleScheduledHTTP(t *testing.T) { name: "invalid path (missing task type)", method: "POST", path: "/api/scheduled/", - setupApp: func(app *Application) {}, + setupApp: func(_ *testing.T, _ *Application) {}, expectedStatus: 400, expectError: false, }, @@ -263,15 +276,8 @@ func TestHandleScheduledHTTP(t *testing.T) { name: "auth required but missing", method: "POST", path: "/api/scheduled/collect_recommendations", - setupApp: func(app *Application) { - v, err := scheduledauth.New(scheduledauth.Config{ - Mode: scheduledauth.ModeBearer, - Bearer: "my-secret", - }) - if err != nil { - panic(err) - } - app.scheduledAuth = v + setupApp: func(t *testing.T, app *Application) { + app.scheduledAuth = newBearerValidator(t, "my-secret") }, expectedStatus: 401, }, @@ -280,15 +286,8 @@ func TestHandleScheduledHTTP(t *testing.T) { method: "POST", path: "/api/scheduled/collect_recommendations", authHeader: "Bearer my-secret", - setupApp: func(app *Application) { - v, err := scheduledauth.New(scheduledauth.Config{ - Mode: scheduledauth.ModeBearer, - Bearer: "my-secret", - }) - if err != nil { - panic(err) - } - app.scheduledAuth = v + setupApp: func(t *testing.T, app *Application) { + app.scheduledAuth = newBearerValidator(t, "my-secret") app.Scheduler = &testutil.MockScheduler{ CollectRecommendationsFunc: func(ctx context.Context) (*scheduler.CollectResult, error) { return &scheduler.CollectResult{}, nil @@ -303,7 +302,7 @@ func TestHandleScheduledHTTP(t *testing.T) { t.Run(tt.name, func(t *testing.T) { app := &Application{} if tt.setupApp != nil { - tt.setupApp(app) + tt.setupApp(t, app) } req := httptest.NewRequest(tt.method, tt.path, nil) diff --git a/internal/server/scheduledauth/errors.go b/internal/server/scheduledauth/errors.go index b1624b0d8..db28d96a1 100644 --- a/internal/server/scheduledauth/errors.go +++ b/internal/server/scheduledauth/errors.go @@ -13,7 +13,7 @@ // Used by Azure Container Apps + Logic Apps where the workflow // pulls the secret from Key Vault at invocation time. // -// See plan: ~/.claude/projects/.../plans/issue-161-oidc-validator.md +// Design context: GitHub PR #161 / issue #159. package scheduledauth import "errors" diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go index c2102c73d..4dd5df315 100644 --- a/internal/server/scheduledauth/validator.go +++ b/internal/server/scheduledauth/validator.go @@ -195,6 +195,12 @@ func (v *Validator) Mode() Mode { return v.mode } +// warmupTimeout is the fallback deadline for the JWKS warmup probe +// when the caller passes a context without one. http.DefaultClient +// has no timeout, so without this guard a misconfigured / unreachable +// JWKS endpoint would block startup indefinitely. +const warmupTimeout = 5 * time.Second + // Warmup performs a best-effort sanity check on the JWKS endpoint at // startup. Failure is non-fatal — it is logged and the validator stays // operable; the underlying *oidc.RemoteKeySet retries on the first real @@ -202,6 +208,10 @@ func (v *Validator) Mode() Mode { // hiccups, while still surfacing misconfiguration (wrong URL, blocked // egress) prominently in the startup logs. // +// If ctx has no deadline, a 5s fallback is applied — startup must +// always make forward progress, even if the caller forgot to bound the +// warmup. Callers that DO want to wait longer are respected. +// // Implementation note: go-oidc's RemoteKeySet does not expose a public // "fetch now" method — it only fetches on demand from VerifySignature // (which requires a parseable JWT). We issue an independent HTTP GET @@ -210,6 +220,11 @@ func (v *Validator) Warmup(ctx context.Context) { if v.mode != ModeOIDC || v.jwksURL == "" { return } + if _, ok := ctx.Deadline(); !ok { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, warmupTimeout) + defer cancel() + } req, err := http.NewRequestWithContext(ctx, http.MethodGet, v.jwksURL, nil) if err != nil { log.Printf("scheduledauth: WARN — JWKS warmup request build failed: %v", err) diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 4af205581..8e2201b1f 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -81,13 +82,18 @@ func newJWKSServer(t *testing.T, body []byte) *jwksServer { _, _ = w.Write(current) }) mux.HandleFunc("/swap", func(w http.ResponseWriter, r *http.Request) { - mu.Lock() - defer mu.Unlock() // Body of POST /swap replaces the served JWKS. Used to test - // refresh-on-unknown-kid. - buf := make([]byte, r.ContentLength) - _, _ = r.Body.Read(buf) + // refresh-on-unknown-kid. io.ReadAll handles short Reads + // correctly — a single Body.Read call may return less than + // ContentLength and silently truncate the JWKS. + buf, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + mu.Lock() current = buf + mu.Unlock() w.WriteHeader(http.StatusOK) }) srv := httptest.NewServer(mux) diff --git a/terraform/environments/gcp/compute.tf b/terraform/environments/gcp/compute.tf index 644c4b64d..7026586fd 100644 --- a/terraform/environments/gcp/compute.tf +++ b/terraform/environments/gcp/compute.tf @@ -50,11 +50,14 @@ module "compute_cloud_run" { vpc_connector_id = module.networking.vpc_connector_id # Scheduled tasks. Per #159 the cloud-run module no longer takes a - # plaintext scheduled_task_secret — Cloud Scheduler authenticates to - # Cloud Run via OIDC token + roles/run.invoker. Application-level - # bearer auth is dropped on GCP (Cloud Run's IAM gate is the - # primary defence; on Azure the bearer check stays because Container - # Apps does not have Cloud Run's built-in OIDC validation). + # plaintext scheduled_task_secret — Cloud Scheduler signs an OIDC + # ID token with the scheduler SA, and the CUDly app validates that + # token at /api/scheduled/* via internal/server/scheduledauth + # (signature, issuer, audience, sub-pin). Cloud Run's IAM gate + # (roles/run.invoker, gated by cloud_run_allow_unauthenticated — + # tracked separately in #78) acts as defence in depth on top. + # Azure stays on bearer + Key Vault because Logic Apps' HTTP + # Connector does not emit Entra OIDC tokens. enable_scheduled_tasks = var.enable_scheduled_tasks recommendation_schedule = var.recommendation_schedule From 985cb8bbbc0b91988a1b38a3d9076d321c7ae871 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 23:14:35 +0200 Subject: [PATCH 07/15] docs(server): clarify scheduled OIDC subject pinning Correct the validator comment to say SCHEDULED_TASK_OIDC_SUBJECTS must contain service-account unique IDs, matching Google ID-token sub claims. --- internal/server/scheduledauth/validator.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go index 4dd5df315..223e3a13f 100644 --- a/internal/server/scheduledauth/validator.go +++ b/internal/server/scheduledauth/validator.go @@ -137,7 +137,10 @@ func configureOIDC(v *Validator, cfg Config) (*Validator, error) { } // Subject pinning is REQUIRED in oidc mode. Any GCP service account // in the same org can mint OIDC tokens with arbitrary `aud` values - // — we MUST also pin the issuer SA email via `sub`. See plan §1. + // — we MUST also pin the issuer SA unique_id via `sub`. Google uses + // the service-account unique_id, not the email address, as the `sub` + // claim for Cloud Scheduler-signed ID tokens. That is what + // SCHEDULED_TASK_OIDC_SUBJECTS must contain. if len(cfg.Subjects) == 0 { return nil, fmt.Errorf("%w: oidc mode requires SCHEDULED_TASK_OIDC_SUBJECTS (defence in depth)", ErrConfigInvalid) } From 6f22b9da2c35a028f608e504151d692ade0e164b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Apr 2026 23:34:36 +0200 Subject: [PATCH 08/15] fix(server): validate scheduled auth JWKS configuration Reject malformed OIDC JWKS URLs during validator construction and verify warmup responses have a minimal JWKS keys array before treating the probe as healthy. --- internal/server/scheduledauth/validator.go | 35 ++++++++++++++++ .../server/scheduledauth/validator_test.go | 42 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go index 223e3a13f..25a4c7094 100644 --- a/internal/server/scheduledauth/validator.go +++ b/internal/server/scheduledauth/validator.go @@ -3,10 +3,13 @@ package scheduledauth import ( "context" "crypto/subtle" + "encoding/json" "errors" "fmt" + "io" "log" "net/http" + "net/url" "strings" "time" @@ -132,6 +135,9 @@ func configureOIDC(v *Validator, cfg Config) (*Validator, error) { if cfg.JWKSURL == "" { cfg.JWKSURL = GoogleJWKSURL } + if err := validateAbsoluteURL(cfg.JWKSURL, "SCHEDULED_TASK_OIDC_JWKS_URL"); err != nil { + return nil, err + } if len(cfg.Audiences) == 0 { return nil, fmt.Errorf("%w: oidc mode requires SCHEDULED_TASK_OIDC_AUDIENCE", ErrConfigInvalid) } @@ -193,6 +199,17 @@ func cleanSet(in []string, label string) (map[string]struct{}, error) { return out, nil } +func validateAbsoluteURL(raw, label string) error { + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("%w: %s must be an absolute URL: %v", ErrConfigInvalid, label, err) + } + if u.Scheme == "" || u.Host == "" { + return fmt.Errorf("%w: %s must be an absolute URL", ErrConfigInvalid, label) + } + return nil +} + // Mode returns the validator's auth mode. Useful for /health. func (v *Validator) Mode() Mode { return v.mode @@ -243,7 +260,25 @@ func (v *Validator) Warmup(ctx context.Context) { if resp.StatusCode >= 400 { log.Printf("scheduledauth: WARN — JWKS warmup got %d from %s "+ "(validator will retry on first request)", resp.StatusCode, v.jwksURL) + return } + if err := validateJWKSBody(resp.Body); err != nil { + log.Printf("scheduledauth: WARN — JWKS warmup got invalid JWKS payload from %s: %v "+ + "(validator will retry on first request)", v.jwksURL, err) + } +} + +func validateJWKSBody(r io.Reader) error { + var doc struct { + Keys []json.RawMessage `json:"keys"` + } + if err := json.NewDecoder(r).Decode(&doc); err != nil { + return err + } + if doc.Keys == nil { + return errors.New(`missing "keys" array`) + } + return nil } // Middleware wraps next with auth enforcement. On failure, writes a 401 diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 8e2201b1f..7bce7c9c4 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -514,6 +514,22 @@ func TestNew_Rejects_OIDCWithoutAudiences(t *testing.T) { } } +func TestNew_RejectsOIDCMalformedJWKSURL(t *testing.T) { + for _, jwksURL := range []string{"://bad", "/relative/path", "https://"} { + t.Run(jwksURL, func(t *testing.T) { + _, err := New(Config{ + Mode: ModeOIDC, + JWKSURL: jwksURL, + Audiences: []string{"https://api.example.com"}, + Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + }) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } + }) + } +} + func TestNew_Rejects_BearerWithoutSecret(t *testing.T) { _, err := New(Config{Mode: ModeBearer}) if !errors.Is(err, ErrConfigInvalid) { @@ -666,6 +682,32 @@ func TestWarmup_HitsJWKSEndpoint(t *testing.T) { } } +func TestValidateJWKSBody_RequiresKeysArray(t *testing.T) { + tests := []struct { + name string + body string + wantErr bool + }{ + {name: "valid", body: `{"keys":[]}`}, + {name: "invalid json", body: `{`, wantErr: true}, + {name: "missing keys", body: `{}`, wantErr: true}, + {name: "null keys", body: `{"keys":null}`, wantErr: true}, + {name: "object keys", body: `{"keys":{}}`, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateJWKSBody(strings.NewReader(tt.body)) + if tt.wantErr && err == nil { + t.Fatalf("expected error") + } + if !tt.wantErr && err != nil { + t.Fatalf("expected nil, got: %v", err) + } + }) + } +} + func TestWarmup_LoggedAndNonFatal_OnDeadEndpoint(t *testing.T) { v, err := New(Config{ Mode: ModeOIDC, From 97253fd9d05615a590d1c3afe397bb95501b5fdf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 00:29:57 +0200 Subject: [PATCH 09/15] fix(security): close scheduled-auth fail-open paths flagged by CR pass 4 (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fail-open hazards CR called out as Major: * config.go: empty SCHEDULED_TASK_AUTH_MODE silently coerced to "disabled". A missed Terraform/Kubernetes env wiring would have booted /api/scheduled/* unauthenticated. LoadConfig now returns ErrConfigInvalid when the env var is unset; "disabled" is reserved for an explicit local-dev opt-in. * gke/main.tf: scheduled_task_auth_mode was tied to var.enable_scheduled_tasks, but kubernetes_ingress_v1.app exposes /api/scheduled/* via the catch-all rule even when the scheduler is off — so disabling the scheduler turned those endpoints into a public unauthenticated trigger. Auth mode is now derived from scheduler SA presence; when no SA is created, the new scheduled_task_auth_mode_override variable decides, with a fail-closed "oidc" default that requires explicit opt-in to "disabled". Plus a test-fixture correctness fix: * validator_test.go: testSchedulerSubject is now the SA unique_id format Google actually puts in the JWT `sub` claim (21-digit numeric), not the SA email. The suite now exercises the real production claim contract. * app_test.go: TestNewApplicationFromDeps now sets SCHEDULED_TASK_AUTH_MODE=disabled explicitly so the new fail-closed LoadConfig accepts the boot. --- internal/server/app_test.go | 6 ++ internal/server/scheduledauth/config.go | 13 +++-- .../server/scheduledauth/validator_test.go | 58 ++++++++++++------- terraform/modules/compute/gcp/gke/main.tf | 20 +++++-- .../modules/compute/gcp/gke/variables.tf | 23 ++++++++ 5 files changed, 88 insertions(+), 32 deletions(-) diff --git a/internal/server/app_test.go b/internal/server/app_test.go index a1311b7af..4de388bca 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -368,6 +368,12 @@ func TestLoadApplicationConfig(t *testing.T) { func TestNewApplicationFromDeps(t *testing.T) { ctx := testutil.TestContext(t) + // SCHEDULED_TASK_AUTH_MODE is required at startup (fail-closed default + // per CR pass-4 review on PR #161). Tests boot the app without + // scheduled-task wiring, so explicitly opt into "disabled" — the same + // thing local-dev tfvars do. + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled") + validDBConfig := &database.Config{ Host: "localhost", Port: 5432, diff --git a/internal/server/scheduledauth/config.go b/internal/server/scheduledauth/config.go index d1561d2c9..242f52b3d 100644 --- a/internal/server/scheduledauth/config.go +++ b/internal/server/scheduledauth/config.go @@ -28,14 +28,17 @@ const ( EnvBearerSecret = "SCHEDULED_TASK_SECRET" // bearer mode only ) -// LoadConfig parses Config from env. Unset SCHEDULED_TASK_AUTH_MODE -// defaults to ModeDisabled (with a startup WARN logged inside New). Any -// other invalid combination returns an ErrConfigInvalid error so the -// server fails fast instead of silently downgrading to no-auth. +// LoadConfig parses Config from env. SCHEDULED_TASK_AUTH_MODE MUST be +// set explicitly to one of "oidc", "bearer", or "disabled" — an unset +// value returns ErrConfigInvalid so a missed Terraform/Kubernetes env +// wiring fails server startup instead of silently booting +// /api/scheduled/* unauthenticated. "disabled" is reserved for an +// explicit local-dev choice and must be opted into deliberately. func LoadConfig(env EnvSource) (Config, error) { mode := strings.ToLower(strings.TrimSpace(env.Get(EnvAuthMode))) if mode == "" { - mode = string(ModeDisabled) + return Config{}, fmt.Errorf("%w: %s is unset (set to oidc, bearer, or disabled)", + ErrConfigInvalid, EnvAuthMode) } cfg := Config{Mode: Mode(mode)} diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 7bce7c9c4..21b06c0da 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -20,6 +20,13 @@ import ( "github.com/go-jose/go-jose/v4/jwt" ) +// testSchedulerSubject is the SA unique_id format Google puts in the +// JWT `sub` claim for SA-signed ID tokens (21-digit numeric). The +// production SCHEDULED_TASK_OIDC_SUBJECTS expects this format, NOT the +// SA email — wiring tests around the email shape would let a "sub-as- +// email" regression slip past the suite. +const testSchedulerSubject = "112233445566778899001" + // testKey wraps an RSA keypair plus the kid we'll publish in the JWKS. type testKey struct { priv *rsa.PrivateKey @@ -153,7 +160,7 @@ func newOIDCValidator(t *testing.T, jwksURL string) *Validator { Issuer: "https://accounts.example.com", JWKSURL: jwksURL, Audiences: []string{"https://api.example.com"}, - Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + Subjects: []string{testSchedulerSubject}, }) if err != nil { t.Fatalf("New: %v", err) @@ -167,7 +174,7 @@ func TestValidate_OIDC_Valid(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) @@ -203,7 +210,7 @@ func TestValidate_OIDC_Expired(t *testing.T) { now := time.Now() claims := baseClaims(now, - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com") // 2 minutes past expiry, well beyond the 60s skew. @@ -222,7 +229,7 @@ func TestValidate_OIDC_ExpiryWithinSkew(t *testing.T) { now := time.Now() claims := baseClaims(now, - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com") // Just expired but within the 60s skew window. @@ -242,7 +249,7 @@ func TestValidate_OIDC_WrongAudience(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://attacker.example.com", "https://accounts.example.com")) @@ -257,7 +264,7 @@ func TestValidate_OIDC_WrongIssuer(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://attacker-iss.com")) @@ -288,7 +295,7 @@ func TestValidate_OIDC_BadSignature(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, other, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) @@ -311,7 +318,7 @@ func TestValidate_OIDC_AlgorithmConfusion(t *testing.T) { // kid="kid-1" is a strictly easier case for the attacker; if the // validator rejects this it covers the core alg-confusion gap. tok := signTokenAlg(t, jose.HS256, []byte("attacker-secret-32-bytes-padding!!"), key.kid, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) @@ -328,7 +335,7 @@ func TestValidate_OIDC_IATInFuture(t *testing.T) { now := time.Now() claims := baseClaims(now, - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com") // 2 minutes in the future, well beyond the 60s skew. @@ -348,7 +355,7 @@ func TestValidate_OIDC_NotBeforeFuture(t *testing.T) { now := time.Now() claims := baseClaims(now, - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com") claims["nbf"] = now.Add(2 * time.Minute).Unix() @@ -369,7 +376,7 @@ func TestValidate_OIDC_AudienceListClaim(t *testing.T) { now := time.Now() claims := baseClaims(now, - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "placeholder", // overridden below "https://accounts.example.com") claims["aud"] = []string{"https://other.example.com", "https://api.example.com"} @@ -391,7 +398,7 @@ func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { v := newOIDCValidator(t, srv.URL) tokA := signToken(t, keyA, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) if err := v.Validate(context.Background(), "Bearer "+tokA); err != nil { @@ -408,7 +415,7 @@ func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { resp.Body.Close() tokB := signToken(t, keyB, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) if err := v.Validate(context.Background(), "Bearer "+tokB); err != nil { @@ -426,7 +433,7 @@ func TestValidate_OIDC_SingleFlight_StampedeProtection(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) @@ -507,7 +514,7 @@ func TestNew_Rejects_OIDCWithoutSubjects(t *testing.T) { func TestNew_Rejects_OIDCWithoutAudiences(t *testing.T) { _, err := New(Config{ Mode: ModeOIDC, - Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + Subjects: []string{testSchedulerSubject}, }) if !errors.Is(err, ErrConfigInvalid) { t.Fatalf("expected ErrConfigInvalid, got: %v", err) @@ -521,7 +528,7 @@ func TestNew_RejectsOIDCMalformedJWKSURL(t *testing.T) { Mode: ModeOIDC, JWKSURL: jwksURL, Audiences: []string{"https://api.example.com"}, - Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + Subjects: []string{testSchedulerSubject}, }) if !errors.Is(err, ErrConfigInvalid) { t.Fatalf("expected ErrConfigInvalid, got: %v", err) @@ -544,13 +551,20 @@ func TestNew_Rejects_UnknownMode(t *testing.T) { } } -func TestLoadConfig_DefaultsToDisabled(t *testing.T) { - cfg, err := LoadConfig(EnvMap{}) +func TestLoadConfig_RejectsMissingAuthMode(t *testing.T) { + _, err := LoadConfig(EnvMap{}) + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid for unset SCHEDULED_TASK_AUTH_MODE, got: %v", err) + } +} + +func TestLoadConfig_ExplicitDisabledIsAccepted(t *testing.T) { + cfg, err := LoadConfig(EnvMap{EnvAuthMode: "disabled"}) if err != nil { - t.Fatalf("LoadConfig: %v", err) + t.Fatalf("LoadConfig(disabled): %v", err) } if cfg.Mode != ModeDisabled { - t.Fatalf("default mode = %s, want %s", cfg.Mode, ModeDisabled) + t.Fatalf("mode = %s, want %s", cfg.Mode, ModeDisabled) } } @@ -621,7 +635,7 @@ func TestMiddleware_OIDC_AllowsAndCallsNextOnSuccess(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "scheduler@example.iam.gserviceaccount.com", + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) @@ -714,7 +728,7 @@ func TestWarmup_LoggedAndNonFatal_OnDeadEndpoint(t *testing.T) { Issuer: "https://accounts.example.com", JWKSURL: "http://127.0.0.1:1/this-port-is-closed", // ECONNREFUSED Audiences: []string{"https://api.example.com"}, - Subjects: []string{"scheduler@example.iam.gserviceaccount.com"}, + Subjects: []string{testSchedulerSubject}, }) if err != nil { t.Fatalf("New: %v", err) diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 54d5bc688..a0e9c442b 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -23,13 +23,23 @@ locals { # internal/server/scheduledauth. Audience pinned to the scheduler SA # email (matches the explicit `audience` on the oidc_token block); # subject pinned to the SA's unique numeric ID (the value Google puts - # in the JWT `sub` claim for SA-signed ID tokens). When the scheduler - # is disabled the env vars become empty strings and the validator - # stays in "disabled" mode (with a startup WARN log) — matches the - # cloud-run module's behaviour. + # in the JWT `sub` claim for SA-signed ID tokens). + # + # Auth mode is derived from scheduler SA presence, NOT from + # var.enable_scheduled_tasks. kubernetes_ingress_v1.app exposes + # /api/scheduled/* via the catch-all rule even when the scheduler is + # disabled, so tying auth to enable_scheduled_tasks would turn a + # disabled scheduler into an unauthenticated public trigger. With no + # SA, var.scheduled_task_auth_mode_override decides — its default is + # the fail-closed "oidc" so an unauthenticated boot requires opting + # in to "disabled" deliberately. scheduled_task_oidc_audience = try(google_service_account.scheduler[0].email, "") scheduled_task_oidc_subject = try(google_service_account.scheduler[0].unique_id, "") - scheduled_task_auth_mode = var.enable_scheduled_tasks ? "oidc" : "disabled" + scheduled_task_auth_mode = ( + length(google_service_account.scheduler) > 0 + ? "oidc" + : var.scheduled_task_auth_mode_override + ) } # ============================================== diff --git a/terraform/modules/compute/gcp/gke/variables.tf b/terraform/modules/compute/gcp/gke/variables.tf index 1331f5fa2..8c3602433 100644 --- a/terraform/modules/compute/gcp/gke/variables.tf +++ b/terraform/modules/compute/gcp/gke/variables.tf @@ -266,3 +266,26 @@ variable "app_url" { type = string default = "" } + +variable "scheduled_task_auth_mode_override" { + description = <<-EOT + Override for SCHEDULED_TASK_AUTH_MODE used ONLY when no scheduler SA + is created (var.enable_scheduled_tasks = false). When the SA exists, + auth mode is always derived as "oidc" — the override is ignored. + + Why this exists: kubernetes_ingress_v1.app exposes /api/scheduled/* + via the catch-all rule even when the scheduler is disabled. Tying + auth to var.enable_scheduled_tasks would silently boot those + endpoints unauthenticated. The fail-closed default ("oidc") here + means a deploy without scheduler SA still requires the validator to + be configured (or it rejects every request). Set this to "disabled" + deliberately for local-dev / dry-run only. + EOT + type = string + default = "oidc" + + validation { + condition = contains(["oidc", "bearer", "disabled"], var.scheduled_task_auth_mode_override) + error_message = "scheduled_task_auth_mode_override must be one of: \"oidc\", \"bearer\", \"disabled\"." + } +} From 5193227c7fa0e0dbce0c4e64f36232fc7727ceba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 00:36:28 +0200 Subject: [PATCH 10/15] test(scheduledauth): finish unique_id fixture migration (CR nitpick residual) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 97253fd9d introduced testSchedulerSubject and converted the positive-path scheduler subjects in validator_test.go to it, but left two clusters of email-shaped fixtures behind: - validator_test.go:282 — the "wrong subject" rejection test still asserted against `attacker@example.iam.gserviceaccount.com`. - integration_test.go (lines 36, 71, 82, 99, 110) — every subject in the integration suite was still email-shaped. The CR nitpick was not just about the positive scheduler fixture: the broader concern is that production wiring pins SCHEDULED_TASK_OIDC_SUBJECTS to GCP service-account unique_ids (21-digit numeric strings), and a regression that silently widened the validator to accept any string would still pass tests that asserted against email-shaped values — both negative and positive paths. This commit: 1. Adds testNonSchedulerSubject ("999888777666555444333") next to the existing testSchedulerSubject. Same shape, distinct numeric so failures are visually traceable. 2. Replaces every remaining email-shaped subject across both test files with the appropriate constant. After this change `grep -E "scheduler@|attacker@|random@|gserviceaccount"` finds zero hits in `internal/server/scheduledauth/`. Tests: `go test ./internal/server/scheduledauth/...` clean; `go test ./...` clean across the full root suite (no other package referenced the email fixtures). --- internal/server/scheduledauth/integration_test.go | 10 +++++----- internal/server/scheduledauth/validator_test.go | 11 ++++++++++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/server/scheduledauth/integration_test.go b/internal/server/scheduledauth/integration_test.go index 34eba032f..7a9c1e143 100644 --- a/internal/server/scheduledauth/integration_test.go +++ b/internal/server/scheduledauth/integration_test.go @@ -33,7 +33,7 @@ func TestIntegration_FullHTTPRoundtrip(t *testing.T) { Issuer: "https://accounts.example.com", JWKSURL: jwksSrv.URL, Audiences: []string{"https://cudly.example.com"}, - Subjects: []string{"scheduler@cudly.iam.gserviceaccount.com"}, + Subjects: []string{testSchedulerSubject}, }) if err != nil { t.Fatalf("New: %v", err) @@ -68,7 +68,7 @@ func TestIntegration_FullHTTPRoundtrip(t *testing.T) { name: "valid token signed by published key", buildAuth: func(t *testing.T) string { return "Bearer " + signToken(t, signingKey, baseClaims(now, - "scheduler@cudly.iam.gserviceaccount.com", + testSchedulerSubject, "https://cudly.example.com", "https://accounts.example.com")) }, @@ -79,7 +79,7 @@ func TestIntegration_FullHTTPRoundtrip(t *testing.T) { name: "token signed by key not in JWKS", buildAuth: func(t *testing.T) string { return "Bearer " + signToken(t, otherKey, baseClaims(now, - "scheduler@cudly.iam.gserviceaccount.com", + testSchedulerSubject, "https://cudly.example.com", "https://accounts.example.com")) }, @@ -96,7 +96,7 @@ func TestIntegration_FullHTTPRoundtrip(t *testing.T) { name: "wrong subject (audience matches but sub does not)", buildAuth: func(t *testing.T) string { return "Bearer " + signToken(t, signingKey, baseClaims(now, - "random@cudly.iam.gserviceaccount.com", + testNonSchedulerSubject, "https://cudly.example.com", "https://accounts.example.com")) }, @@ -107,7 +107,7 @@ func TestIntegration_FullHTTPRoundtrip(t *testing.T) { name: "wrong audience (sub matches but aud does not)", buildAuth: func(t *testing.T) string { return "Bearer " + signToken(t, signingKey, baseClaims(now, - "scheduler@cudly.iam.gserviceaccount.com", + testSchedulerSubject, "https://attacker.example.com", "https://accounts.example.com")) }, diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 21b06c0da..7af057ebc 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -27,6 +27,15 @@ import ( // email" regression slip past the suite. const testSchedulerSubject = "112233445566778899001" +// testNonSchedulerSubject is a stand-in for any non-listed SA's unique_id, +// used by negative-path tests that verify the validator rejects subjects +// outside SCHEDULED_TASK_OIDC_SUBJECTS. Same shape as testSchedulerSubject +// (21-digit numeric) — using an email-shaped fixture in the rejection +// tests would have made them pass for the wrong reason (subject +// mismatch by FORMAT, not by VALUE), masking a regression that +// silently widened the validator to accept any string. +const testNonSchedulerSubject = "999888777666555444333" + // testKey wraps an RSA keypair plus the kid we'll publish in the JWKS. type testKey struct { priv *rsa.PrivateKey @@ -279,7 +288,7 @@ func TestValidate_OIDC_WrongSubject(t *testing.T) { v := newOIDCValidator(t, srv.URL) tok := signToken(t, key, baseClaims(time.Now(), - "attacker@example.iam.gserviceaccount.com", + testNonSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) From e993d67e4e5ca325cc16c31d11a71b7f01612b69 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 00:45:33 +0200 Subject: [PATCH 11/15] test(scheduledauth): assert /swap status in JWKS rotation test (CR pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's actionable on validator_test.go:418-424: the JWKS-rotation test fired POST /swap and only checked the network-error path. If the test server's /swap handler 5xx'd, or short-read the body and returned non-200 silently, the JWKS would not actually swap and the test would later fail at "unknown kid" — pointing at the wrong layer. Fixes: - Hoist `jwks(keyB)` into a local so we don't compute it three times for body+ContentLength+failure-message symmetry. - Surface NewRequest's error instead of silently dropping it via `_, _ := http.NewRequest(...)`. - Use `http.MethodPost` + `http.StatusOK` instead of stringly- typed literals. - On non-200, read the response body and t.Fatalf with both status and body so the failure points at the swap server, not the downstream validator. Tests: TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid passes; `go test ./...` clean across the full root suite. --- .../server/scheduledauth/validator_test.go | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 7af057ebc..b0fa5b9f5 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -415,12 +415,28 @@ func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { } // Swap the JWKS to publish kid B. - swap, _ := http.NewRequest("POST", srv.URL+"/swap", strings.NewReader(string(jwks(keyB)))) - swap.ContentLength = int64(len(jwks(keyB))) + // + // Both the request build and the response status are checked: if the + // /swap handler 5xx's (or — more subtly — returns a non-200 because + // the body short-read), the JWKS would silently NOT update. The test + // would then fail later at "unknown kid" instead of pointing at the + // real cause. Surfacing the swap failure here keeps the diagnostic + // chain short. + jwksB := jwks(keyB) + swap, err := http.NewRequest(http.MethodPost, srv.URL+"/swap", strings.NewReader(string(jwksB))) + if err != nil { + t.Fatalf("build swap request: %v", err) + } + swap.ContentLength = int64(len(jwksB)) resp, err := http.DefaultClient.Do(swap) if err != nil { t.Fatalf("swap: %v", err) } + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + t.Fatalf("swap: unexpected status %d, body: %s", resp.StatusCode, string(body)) + } resp.Body.Close() tokB := signToken(t, keyB, baseClaims(time.Now(), From b473ebea3d66d17701f459474514f08456ec3d20 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 01:26:12 +0200 Subject: [PATCH 12/15] fix(terraform/gcp): bind scheduled-task OIDC `aud` to endpoint, not SA (CR pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's nitpick on `gke/main.tf:36-37` (also-applies to :480-483 and :714-727), and applies the same convention to the sibling `cloud-run/main.tf` so the two GCP compute modules stay aligned mid-PR. Old shape (both modules): audience = google_service_account.scheduler[0].email # JWT claim SCHEDULED_TASK_OIDC_AUDIENCE = same SA email `sub` already pins the caller to the scheduler SA's unique_id, so using the same SA identity for `aud` made both checks describe the caller. If the scheduler SA were ever reused for a different service, an OIDC token minted for service A could be replayed against service B because both services would have the same audience in their allow-list. New shape (audience names the receiving endpoint, not the caller): GKE: audience = ${var.app_url}/api/scheduled/recommendations Cloud Run: audience = ${var.service_name}/api/scheduled/ The validator side already supports comma-separated audiences via `cleanSet` / `audienceMatches` (no app-side change needed). Both sides read the same local, so the JWT claim and the env-var match by construction. Why service_name (not URL) on Cloud Run: Using `google_cloud_run_v2_service.main.uri` produces a Terraform cycle — the service's env vars include the audience whitelist, which would then depend on the service's own URI. `service_name` is a module input, fully resolvable at plan time, and any unique-per-(service, endpoint) string satisfies the OIDC recipient-bound contract — the validator only does string equality against the env-var value, which is built from the same local. GKE has no such cycle (kubernetes_deployment doesn't read its own URL into env), so it uses `var.app_url` per CR's literal suggestion. Per-module specifics: * gke/main.tf — single endpoint; one local, three callsites (the local + the env var + the Cloud Scheduler oidc_token block) all read from `local.scheduled_task_oidc_audience`. * cloud-run/main.tf — two endpoints (recommendations, ri-exchange); new per-schedule locals `scheduled_task_recommendations_audience` and `scheduled_task_ri_exchange_audience` build from `var.service_name`. The CSV `oidc_audiences` local that flows into the env-var compacts the two enabled values. Each Cloud Scheduler oidc_token reads its matching local. Verified: `terraform validate` clean on both modules; pre-commit hooks (`terraform_fmt`, `terraform_validate`, `tflint`) pass; `go test ./...` clean across the full root suite. --- .../modules/compute/gcp/cloud-run/main.tf | 44 ++++++++++++++----- terraform/modules/compute/gcp/gke/main.tf | 18 +++++--- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 74ea5ce74..686d1404e 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -49,9 +49,29 @@ resource "google_service_account" "cloud_run" { # we use try() to short-circuit). `join(",")` produces the CSV format # the validator parses. locals { + # Per-schedule audiences: each Cloud Scheduler job mints an OIDC + # token whose `aud` claim names the receiving endpoint, NOT the + # scheduler SA. `sub` already pins the caller to the SA's unique_id; + # using the SA email for `aud` made both checks describe the caller + # and was replay-prone if the SA was ever reused for a different + # service. The new shape keeps the token recipient-bound. (CodeRabbit + # nitpick on PR #161.) + # + # We deliberately use ${var.service_name}/api/scheduled/ + # rather than the Cloud Run URL: the URL would create a Terraform + # cycle (google_cloud_run_v2_service.main.uri → env vars on the + # service itself → the audience locals → back to the service). + # `service_name` is a module input, so the audience is fully + # resolvable before the Cloud Run resource is planned. Any string + # that's unique-per-(service, endpoint) satisfies the OIDC + # recipient-bound contract — the validator only does string-equality + # against this same value (built once into the env var below). + scheduled_task_recommendations_audience = "${var.service_name}/api/scheduled/recommendations" + scheduled_task_ri_exchange_audience = "${var.service_name}/api/scheduled/ri-exchange" + scheduled_task_oidc_audiences = join(",", compact([ - var.enable_scheduled_tasks ? try(google_service_account.scheduler[0].email, "") : "", - var.enable_ri_exchange_schedule ? try(google_service_account.scheduler_ri_exchange[0].email, "") : "", + var.enable_scheduled_tasks ? local.scheduled_task_recommendations_audience : "", + var.enable_ri_exchange_schedule ? local.scheduled_task_ri_exchange_audience : "", ])) scheduled_task_oidc_subjects = join(",", compact([ var.enable_scheduled_tasks ? try(google_service_account.scheduler[0].unique_id, "") : "", @@ -312,17 +332,17 @@ resource "google_cloud_scheduler_job" "recommendations" { # 2. App-level OIDC validation on /api/scheduled/* — the Go # validator (internal/server/scheduledauth) checks the JWT # signature, issuer, audience, and pins the subject to this - # SA. Pinning `audience` here explicitly to the SA email - # makes the validator's allow-list deterministic; without an - # explicit value Cloud Scheduler defaults audience to the - # target URL, which would couple #159's fix to the Cloud Run - # URL shape. + # SA's unique_id. Audience is pinned to the receiving endpoint + # URL (the OAuth/OIDC convention — keeps the token recipient- + # bound). The validator's whitelist reads the same value via + # local.scheduled_task_oidc_audiences so the JWT claim and + # the env-var match by construction. # The previous static `Authorization: Bearer # ${var.scheduled_task_secret}` header leaked the shared secret into # the scheduler resource definition + Terraform state — closes #159. oidc_token { service_account_email = google_service_account.scheduler[0].email - audience = google_service_account.scheduler[0].email + audience = local.scheduled_task_recommendations_audience } } } @@ -371,11 +391,13 @@ resource "google_cloud_scheduler_job" "ri_exchange" { uri = "${google_cloud_run_v2_service.main.uri}/api/scheduled/ri-exchange" # Same OIDC-only model as the recommendations scheduler above — - # see #159 for the rationale. Audience pinned to this SA email so - # the app-level validator's allow-list stays deterministic. + # see #159 for the rationale. Audience pinned to the receiving + # endpoint URL so the token is recipient-bound and the validator's + # whitelist is matched by construction (the env var includes this + # value via local.scheduled_task_oidc_audiences). oidc_token { service_account_email = google_service_account.scheduler_ri_exchange[0].email - audience = google_service_account.scheduler_ri_exchange[0].email + audience = local.scheduled_task_ri_exchange_audience } } } diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index a0e9c442b..5fa608c73 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -33,7 +33,13 @@ locals { # SA, var.scheduled_task_auth_mode_override decides — its default is # the fail-closed "oidc" so an unauthenticated boot requires opting # in to "disabled" deliberately. - scheduled_task_oidc_audience = try(google_service_account.scheduler[0].email, "") + # Audience is bound to the receiving endpoint, NOT to the scheduler + # service-account identity. `sub` already pins the caller to that SA's + # unique_id; using the same SA identity for `aud` made both checks + # describe the caller, which is replay-prone if the SA is ever reused. + # Pinning `aud` to the endpoint URL keeps tokens recipient-bound — the + # OAuth/OIDC convention. (CodeRabbit nitpick on PR #161.) + scheduled_task_oidc_audience = "${var.app_url}/api/scheduled/recommendations" scheduled_task_oidc_subject = try(google_service_account.scheduler[0].unique_id, "") scheduled_task_auth_mode = ( length(google_service_account.scheduler) > 0 @@ -716,15 +722,17 @@ resource "google_cloud_scheduler_job" "recommendations" { # IAM gate, so the OIDC token is validated by the app at # /api/scheduled/* via internal/server/scheduledauth — signature, # issuer, audience, and `sub` pinned to this SA. Audience is set - # explicitly here so the validator's allow-list stays - # deterministic; without it Cloud Scheduler defaults audience to - # the target URL. + # to the receiving endpoint URL (the OAuth/OIDC convention — keeps + # the token recipient-bound and limits replay if this SA is ever + # reused for a different service). The same value is exported to + # the app via local.scheduled_task_oidc_audience so the validator's + # whitelist matches by construction. # The previous static `Authorization: Bearer # ${var.scheduled_task_secret}` header leaked the shared secret into # the scheduler resource definition + Terraform state — closes #159. oidc_token { service_account_email = google_service_account.scheduler[0].email - audience = google_service_account.scheduler[0].email + audience = local.scheduled_task_oidc_audience } } } From 171eea5f454ac6f9fa219b213432eae08c920cc1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 01:26:38 +0200 Subject: [PATCH 13/15] test(scheduledauth): assert warmup failure-path log fires (CR pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's nitpick on `validator_test.go:750-766`: `TestWarmup_LoggedAndNonFatal_OnDeadEndpoint` previously only asserted that Warmup didn't hang. The test would still pass if a future regression made Warmup short-circuit *before* the JWKS fetch ever ran — exactly the failure mode the test exists to guard against ("non-fatal, but actually exercises the failure branch"). This commit captures the global standard logger to a `bytes.Buffer` for the duration of the test, calls Warmup with a guaranteed-bad JWKS URL (127.0.0.1:1, ECONNREFUSED), and asserts the captured output contains "JWKS warmup". That string is shared by every warmup-failure log line in `validator.go` (request build, fetch, non-200 status, invalid payload), so a tweak to any of those messages won't break this test for cosmetic reasons — but a regression that skips the fetch entirely will. Safety: no test in `internal/server/scheduledauth/` uses `t.Parallel()`, so swapping the global `log` writer is race-free. `t.Cleanup` restores the original writer. Tests: `go test ./internal/server/scheduledauth/... -run Warmup` shows the new assertion firing on success; `go test ./...` clean across the full root suite. --- .../server/scheduledauth/validator_test.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index b0fa5b9f5..d35a61241 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -1,6 +1,7 @@ package scheduledauth import ( + "bytes" "context" "crypto/rand" "crypto/rsa" @@ -8,6 +9,7 @@ import ( "errors" "fmt" "io" + "log" "net/http" "net/http/httptest" "strings" @@ -748,6 +750,17 @@ func TestValidateJWKSBody_RequiresKeysArray(t *testing.T) { } func TestWarmup_LoggedAndNonFatal_OnDeadEndpoint(t *testing.T) { + // Capture the global logger so we can assert the failure path was + // actually exercised. Without this, the test passes just as well if + // Warmup were to short-circuit before the fetch ever ran (CR pass on + // PR #161 — "did not hang" alone doesn't pin the non-fatal branch). + // No tests in this package use t.Parallel(), so swapping the global + // log writer here is safe; t.Cleanup restores it. + var logBuf bytes.Buffer + origOut := log.Writer() + log.SetOutput(&logBuf) + t.Cleanup(func() { log.SetOutput(origOut) }) + v, err := New(Config{ Mode: ModeOIDC, Issuer: "https://accounts.example.com", @@ -763,6 +776,17 @@ func TestWarmup_LoggedAndNonFatal_OnDeadEndpoint(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() v.Warmup(ctx) // non-fatal — only logs + + // Assert the failure-path log fired. The validator emits + // "scheduledauth: WARN — JWKS warmup fetch failed for : " + // on connect refused (validator.go:255). We pin on the substring + // "JWKS warmup" so a future log-message tweak doesn't break the + // test for cosmetic reasons, while still proving the code path + // reached the post-fetch error branch. + logs := logBuf.String() + if !strings.Contains(logs, "JWKS warmup") { + t.Fatalf("expected a JWKS-warmup failure log, got: %q", logs) + } } // guard: sanity-check that Mode is reported back. From 7dbf8596c2fb18c7fa6851298ff967c4b596c80e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 01:35:18 +0200 Subject: [PATCH 14/15] refactor(terraform/gke): tighten scheduled-auth comment + share endpoint local (CR pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CR nitpicks on `gke/main.tf`: 1. **Line 22-41 stale comment**: the comment block introducing the scheduled_task_oidc_* locals still described the OLD SA-email audience model in its first paragraph and then contradicted itself in its second. Rewritten as a single coherent explanation: `aud` names the recipient (endpoint URL), `sub` names the caller (SA unique_id, not email — the form Google actually puts in the JWT `sub` claim). 2. **Lines 718-735 string duplication**: the Cloud Scheduler `http_target.uri` and `oidc_token.audience` were both typed as `${var.app_url}/api/scheduled/recommendations`, i.e. the same string twice. CR's concern: if one side changed without the other, Cloud Scheduler would mint tokens with `aud` for the old endpoint while POSTing to the new one — silent 401. Both now read `local.scheduled_task_oidc_audience`, so a future endpoint rename becomes a syntax-level mismatch rather than a runtime auth failure. Note: the same collapse does NOT apply to `cloud-run/main.tf` because on that side the `uri` (real Cloud Run service URL, `google_cloud_run_v2_service.main.uri/...`) and the `audience` (service_name-prefixed string) are intentionally different values — the audience uses `var.service_name` to break the Terraform dependency cycle that the URL would create. Different values by design, no drift risk. Verified: `terraform validate` clean on the gke module; `go test ./...` clean across the root suite. --- terraform/modules/compute/gcp/gke/main.tf | 36 ++++++++++++----------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 5fa608c73..f193944d3 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -20,10 +20,17 @@ locals { ) # Scheduled-task OIDC validator wiring — see - # internal/server/scheduledauth. Audience pinned to the scheduler SA - # email (matches the explicit `audience` on the oidc_token block); - # subject pinned to the SA's unique numeric ID (the value Google puts - # in the JWT `sub` claim for SA-signed ID tokens). + # internal/server/scheduledauth. + # + # `aud` (audience) names the RECEIVING endpoint — the URL Cloud + # Scheduler is calling. Pinning aud to the endpoint keeps tokens + # recipient-bound (the OAuth/OIDC convention) and limits cross-service + # replay if the scheduler SA is ever reused. + # + # `sub` (subject) names the CALLER — the scheduler SA's unique + # numeric ID. Google puts the SA unique_id (not its email) in the + # JWT `sub` claim for SA-signed ID tokens, so the validator's + # SCHEDULED_TASK_OIDC_SUBJECTS env var must contain that numeric form. # # Auth mode is derived from scheduler SA presence, NOT from # var.enable_scheduled_tasks. kubernetes_ingress_v1.app exposes @@ -33,12 +40,6 @@ locals { # SA, var.scheduled_task_auth_mode_override decides — its default is # the fail-closed "oidc" so an unauthenticated boot requires opting # in to "disabled" deliberately. - # Audience is bound to the receiving endpoint, NOT to the scheduler - # service-account identity. `sub` already pins the caller to that SA's - # unique_id; using the same SA identity for `aud` made both checks - # describe the caller, which is replay-prone if the SA is ever reused. - # Pinning `aud` to the endpoint URL keeps tokens recipient-bound — the - # OAuth/OIDC convention. (CodeRabbit nitpick on PR #161.) scheduled_task_oidc_audience = "${var.app_url}/api/scheduled/recommendations" scheduled_task_oidc_subject = try(google_service_account.scheduler[0].unique_id, "") scheduled_task_auth_mode = ( @@ -715,18 +716,19 @@ resource "google_cloud_scheduler_job" "recommendations" { http_target { http_method = "POST" - uri = "${var.app_url}/api/scheduled/recommendations" + # Both the request target and the OIDC audience read the same + # local so they cannot drift. If they ever did, Cloud Scheduler + # would mint a token with `aud` for the old endpoint while + # actually POSTing to the new one — the validator would 401 + # silently. Sharing the local makes that class of regression a + # syntax-level mismatch rather than a runtime auth failure. + uri = local.scheduled_task_oidc_audience # Auth: oidc_token below is signed by the scheduler's service # account at invocation time. GKE Ingress has no Cloud-Run-style # IAM gate, so the OIDC token is validated by the app at # /api/scheduled/* via internal/server/scheduledauth — signature, - # issuer, audience, and `sub` pinned to this SA. Audience is set - # to the receiving endpoint URL (the OAuth/OIDC convention — keeps - # the token recipient-bound and limits replay if this SA is ever - # reused for a different service). The same value is exported to - # the app via local.scheduled_task_oidc_audience so the validator's - # whitelist matches by construction. + # issuer, audience, and `sub` pinned to this SA's unique_id. # The previous static `Authorization: Bearer # ${var.scheduled_task_secret}` header leaked the shared secret into # the scheduler resource definition + Terraform state — closes #159. From 7935b6be6d58bc9a7678d9b34c6f5b3112cb2c48 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 01:55:18 +0200 Subject: [PATCH 15/15] docs(terraform/azure): document recommendation_schedule's hour-only parsing (CR pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's open Major on `dev.tfvars.example:90`: the example was tagged "# Cron: Daily at 2 AM" which read as "full cron syntax supported". A user copying it and changing to e.g. `"*/15 * * * *"` would silently get a daily-at-00:00 schedule instead, because the consuming code in `modules/compute/azure/container-apps/scheduled-tasks.tf:23` only splits on spaces and reads index [1] (the hour field). CR's literal suggestion was to switch the example to a frequency/ interval form. Doing that would diverge the Azure tfvars files from the AWS/GCP ones (which all use the same cron string) and would require a parser change. Both are bigger than the actual finding. The minimal fix is to be honest about the limitation in the docs. Three sites updated: 1. `terraform/environments/azure/dev.tfvars.example:78-90` — replaced the `# Cron: Daily at 2 AM` one-liner with a comment block explicitly stating that only the hour field is honored, pointing at the parser, and warning that sub-daily / DoW / DoM patterns are silently ignored. 2. `terraform/environments/azure/variables.tf:380-390` — variable description rewritten to the same effect, so `terraform plan` output and module docs reflect reality. 3. `terraform/modules/compute/azure/container-apps/variables.tf:225-235` — module-level variable description updated symmetrically so module consumers get the same warning. No behavioural change. Verified `terraform validate` clean on `terraform/environments/azure` and `terraform/modules/compute/azure/container-apps`. If finer-grained scheduling becomes a real requirement, the follow-up is to extend the parser and switch the variables to something like `recommendation_schedule_frequency` + `_interval` / `_start_time`, then drop the cron-shaped string. That's intentionally out of scope here. --- terraform/environments/azure/dev.tfvars.example | 12 ++++++++++-- terraform/environments/azure/variables.tf | 10 ++++++++-- .../compute/azure/container-apps/variables.tf | 9 ++++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/terraform/environments/azure/dev.tfvars.example b/terraform/environments/azure/dev.tfvars.example index cc0f1ffce..1734e03fa 100644 --- a/terraform/environments/azure/dev.tfvars.example +++ b/terraform/environments/azure/dev.tfvars.example @@ -86,8 +86,16 @@ key_vault_default_network_acl_action = "Allow" # itself is wired through `module.secrets.scheduled_task_secret_name` # and never lives in the container env. -enable_scheduled_tasks = true -recommendation_schedule = "0 2 * * *" # Cron: Daily at 2 AM +enable_scheduled_tasks = true + +# recommendation_schedule: a cron-shaped string, but ONLY the hour field +# (index [1]) is honored — the Azure module converts it into a Logic +# Apps recurrence trigger of frequency=Day + interval=1 starting at +# that hour (see modules/compute/azure/container-apps/scheduled-tasks.tf). +# Sub-daily / day-of-week / day-of-month patterns are silently ignored. +# Stick to daily-at-hour-N values; if you need finer granularity, +# extend the parser before pinning a different schedule. +recommendation_schedule = "0 2 * * *" # Daily at 02:00 UTC # ============================================== # Frontend (Azure CDN) diff --git a/terraform/environments/azure/variables.tf b/terraform/environments/azure/variables.tf index df517c6f9..dc26f8570 100644 --- a/terraform/environments/azure/variables.tf +++ b/terraform/environments/azure/variables.tf @@ -378,9 +378,15 @@ variable "enable_scheduled_tasks" { } variable "recommendation_schedule" { - description = "Cron schedule for recommendations task" + description = <<-EOT + Cron-shaped schedule for the recommendations Logic App. Only the + hour field is parsed (the Azure module converts it to Logic Apps + frequency=Day + interval=1 + startTime=:00). Sub-daily, + day-of-week, and day-of-month patterns are silently ignored; + use daily-at-hour-N values only. + EOT type = string - default = "0 2 * * *" # 2 AM daily + default = "0 2 * * *" # Daily at 02:00 UTC } variable "enable_ri_exchange_schedule" { diff --git a/terraform/modules/compute/azure/container-apps/variables.tf b/terraform/modules/compute/azure/container-apps/variables.tf index b493154ac..354f669bd 100644 --- a/terraform/modules/compute/azure/container-apps/variables.tf +++ b/terraform/modules/compute/azure/container-apps/variables.tf @@ -223,7 +223,14 @@ variable "scheduled_task_secret_name" { } variable "recommendation_schedule" { - description = "Cron schedule for recommendations refresh (default: daily at 2 AM UTC)" + description = <<-EOT + Cron-shaped schedule for the recommendations Logic App. Only the + hour field (index [1]) is parsed — see scheduled-tasks.tf — and + the value is materialized as a frequency=Day + interval=1 + recurrence starting at that hour. Sub-daily, day-of-week, and + day-of-month cron patterns are silently ignored; stick to + daily-at-hour-N values. Default: daily at 02:00 UTC. + EOT type = string default = "0 2 * * *" }