feat: auth & access - SSO support, TLS, and signing-key rotation (FB-896)

[kop]

Summary

Implements FB-896: engine-side authentication/SSO, full TLS (engine-listener + gateway upstream/downstream), and operator-owned JWT signing-key rotation.

• Auth core (8daa31c): spec.auth on FireboltInstance — native/OIDC authorization servers, operator-provisioned admin credentials wiring and JWT signing key via cert-manager, rendered into every engine's instance.auth.* per packdb's closed config schema.
• Engine-listener TLS (12d6e17): spec.tls.engine terminates TLS on each engine's query listener; the gateway re-encrypts its upstream connection to engines using the same CA.
• Gateway downstream TLS (4292941): spec.tls.gateway terminates TLS on the gateway's client-facing Envoy listener, with matching probe-scheme and cert provisioning.
• Signing-key rotation (e8dcaf0): optional signingKeys.rotationInterval/retainDuration drive a two-phase mint → promote → retain → remove state machine, gated at every step on a new FireboltEngineStatus.ObservedAuthHash field so a key is never promoted or removed until every engine has actually converged on the config that makes that step safe. The retain window is anchored at confirmed post-promotion convergence rather than the promotion decision, closing a validation-gap window a naive decision-time anchor would reopen.

All crypto material (signing keys, TLS certs) is provisioned via cert-manager Certificates against a user-supplied Issuer/ClusterIssuer — no bring-your-own-secret option, matching this operator's existing TLS design.

Known limitations (carried forward, not new)

• No live TLS-handshake or JWT-issuance e2e test was run against a real firebolt-core engine — covered by unit/golden-file tests and cert-manager-backed e2e infra only for the apply/issue path.
• DefaultEngineWebImage's CA-trust behavior when proxying to https://localhost:3473 with uiSidecar: true + engine TLS enabled is unverified against the real image.

Test plan

• go build ./..., go vet ./..., go test ./... all clean
• make lint clean (0 issues)
• make manifests generate re-run confirmed idempotent
• New unit tests for the full rotation lifecycle (mint/promote/retain/remove), including a composition test that a lagging engine concretely blocks both the promote and retire-eligible gates
• Discriminating-test verification on the retain-window anchor fix (confirmed the test fails without the fix, passes with it)
• e2e TLS handshake / JWT issuance against a real engine (not run — see known limitations)

---

Note

High Risk
Large CRD and behavioral change across authentication, TLS termination, and coordinated JWT signing-key rotation; misconfiguration or rotation bugs could break logins, cross-engine token validation, or TLS paths before engines start.

Overview
Replaces the placeholder spec.auth (mode + OAuth client-style OIDC) with an instance-wide, packdb-shaped auth model: enabled, embedded local login (admin password via Secret mount, cert-manager JWT signing keys), and multi-provider OIDC bearer validation (discovery URL, username mapping, JIT, JWKS/discovery TTLs). Adds spec.tls for gateway client-facing and engine listeners, with certificates only via cert-manager IssuerRef (no BYO Secret).

Status and rollout: status.auth tracks signing keys and a rotation state machine (Active / ValidationOnly / Removing, RetireEligibleAt); optional rotationInterval / retainDuration are validated at admission. FireboltEngine.status.observedAuthHash exposes the stable auth config hash so the instance controller can gate promote/retire on every engine converging. New AuthReady, EngineTLSReady, and GatewayTLSReady conditions are documented as not folded into top-level Instance Ready.

Operator surface: Reserved engine/gateway volumes for admin creds, per-kid signing secrets (auth-signing-* prefix), and TLS; instance.auth and endpoints join operator-owned engine config paths. Webhook adds ValidateAuth / ValidateTLS (re-runnable at reconcile), packdb duration parsing (1d etc.), and broad unit tests. Runtime wiring: cert-manager API on the scheme, Certificate RBAC, dependency and regenerated CRD/Helm JSON schema updates, and an expanded instance-full example.

^{Reviewed by Cursor Bugbot for commit e8dcaf0. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Negative JWT durations pass admission
  • FireboltInstance auth validation now rejects negative JWT durations, including maxTokenAge values that could bypass signing-key retain safety checks.

Or push these changes by commenting:

@cursor push 7dbede6aa7

Preview (7dbede6aa7)

diff --git a/api/v1alpha1/fireboltinstance_webhook.go b/api/v1alpha1/fireboltinstance_webhook.go
--- a/api/v1alpha1/fireboltinstance_webhook.go
+++ b/api/v1alpha1/fireboltinstance_webhook.go
@@ -245,7 +245,7 @@
 // validateLocalAuth checks the admin-password reference and signing-key
 // policy that Enabled=true requires, the chosen JWT signing algorithm's
 // compatibility with the signing key's cert-manager algorithm, and the
-// format of the embedded server's optional JWT duration fields.
+// validity of the embedded server's optional JWT duration fields.
 func validateLocalAuth(local *LocalAuthSpec, base *field.Path) field.ErrorList {
 	var errs field.ErrorList
 
@@ -269,13 +269,13 @@
 		errs = append(errs, validateSigningKeyRotation(local, base.Child("signingKeys"))...)
 	}
 
-	if err := validateDurationField(base.Child("tokenExpiry"), local.TokenExpiry); err != nil {
+	if err := validateNonNegativeDurationField(base.Child("tokenExpiry"), local.TokenExpiry); err != nil {
 		errs = append(errs, err)
 	}
-	if err := validateDurationField(base.Child("maxTokenAge"), local.MaxTokenAge); err != nil {
+	if err := validateNonNegativeDurationField(base.Child("maxTokenAge"), local.MaxTokenAge); err != nil {
 		errs = append(errs, err)
 	}
-	if err := validateDurationField(base.Child("clockSkewTolerance"), local.ClockSkewTolerance); err != nil {
+	if err := validateNonNegativeDurationField(base.Child("clockSkewTolerance"), local.ClockSkewTolerance); err != nil {
 		errs = append(errs, err)
 	}
 
@@ -293,10 +293,10 @@
 
 	if oidc.JWT != nil {
 		jwtPath := base.Child("jwt")
-		if err := validateDurationField(jwtPath.Child("clockSkewTolerance"), oidc.JWT.ClockSkewTolerance); err != nil {
+		if err := validateNonNegativeDurationField(jwtPath.Child("clockSkewTolerance"), oidc.JWT.ClockSkewTolerance); err != nil {
 			errs = append(errs, err)
 		}
-		if err := validateDurationField(jwtPath.Child("maxTokenAge"), oidc.JWT.MaxTokenAge); err != nil {
+		if err := validateNonNegativeDurationField(jwtPath.Child("maxTokenAge"), oidc.JWT.MaxTokenAge); err != nil {
 			errs = append(errs, err)
 		}
 	}
@@ -365,6 +365,22 @@
 	return nil
 }
 
+// validateNonNegativeDurationField is validateDurationField plus a
+// non-negativity check for JWT durations whose defaults are zero or greater.
+func validateNonNegativeDurationField(path *field.Path, value string) *field.Error {
+	if value == "" {
+		return nil
+	}
+	d, err := parsePackdbDuration(value)
+	if err != nil {
+		return field.Invalid(path, value, `must be a valid duration string (e.g. "30s", "1h", "1d")`)
+	}
+	if d < 0 {
+		return field.Invalid(path, value, "must not be negative")
+	}
+	return nil
+}
+
 // validatePositiveDurationField is validateDurationField plus a positivity
 // check, for the one duration field (oidc.providers[].discovery.refreshInterval)
 // packdb itself rejects when non-positive.

diff --git a/api/v1alpha1/fireboltinstance_webhook_test.go b/api/v1alpha1/fireboltinstance_webhook_test.go
--- a/api/v1alpha1/fireboltinstance_webhook_test.go
+++ b/api/v1alpha1/fireboltinstance_webhook_test.go
@@ -520,6 +520,18 @@
 			wantError: true,
 		},
 		{
+			name: "negative local jwt duration is rejected",
+			auth: &AuthSpec{
+				Enabled: true,
+				Local: &LocalAuthSpec{
+					Admin:       validAdminSpec(),
+					SigningKeys: validSigningKeys(),
+					TokenExpiry: "-1h",
+				},
+			},
+			wantError: true,
+		},
+		{
 			name: "oidc.jwt duration fields validated the same way as local's",
 			auth: &AuthSpec{
 				Enabled: true,
@@ -534,6 +546,20 @@
 			wantError: true,
 		},
 		{
+			name: "negative oidc jwt duration is rejected",
+			auth: &AuthSpec{
+				Enabled: true,
+				Local:   &LocalAuthSpec{Admin: validAdminSpec(), SigningKeys: validSigningKeys()},
+				OIDC: &OIDCAuthSpec{
+					JWT: &OIDCJWTSpec{MaxTokenAge: "-1d"},
+					Providers: []OIDCProviderSpec{
+						{Name: "okta", DiscoveryURL: "https://okta.example.com/.well-known/openid-configuration", UsernameMapping: "{{ email }}"},
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
 			name: "oidc provider jwks.cacheTTL and discovery.refreshInterval accept packdb's days unit",
 			auth: &AuthSpec{
 				Enabled: true,
@@ -637,6 +663,22 @@
 			wantError: true,
 		},
 		{
+			name: "negative maxTokenAge cannot lower signing key retain floor",
+			auth: &AuthSpec{
+				Enabled: true,
+				Local: &LocalAuthSpec{
+					Admin:       validAdminSpec(),
+					MaxTokenAge: "-1d",
+					SigningKeys: &SigningKeyPolicy{
+						CertManager:      validSigningKeys().CertManager,
+						RotationInterval: &metav1.Duration{Duration: 30 * 24 * time.Hour},
+						RetainDuration:   &metav1.Duration{Duration: time.Hour},
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
 			name: "rotationInterval with retainDuration at least maxTokenAge is valid",
 			auth: &AuthSpec{
 				Enabled: true,

_{You can send follow-ups to the cloud agent here.}

^{Reviewed by Cursor Bugbot for commit e8dcaf0. Configure here.}

[cursor]

Stale comment

[cursor]

Stale comment

[cursor]

Stale comment

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, and Cursor Security Agent left unresolved medium-severity findings (gateway TLS secret/key exposure and TLS downgrade window). Assigned gm42 and fstr for human review.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, and Cursor Security Agent left unresolved medium-severity findings (gateway TLS secret/key exposure and TLS downgrade window). Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[cursor]

Stale comment

[cursor]

🔒 Agentic Security Review
Severity: HIGH

validateReservedVolumeMounts blocks sidecar/init mounts only by volume name, but it does not block user-defined pod volumes that reference operator-managed Secrets by secretName. A template author can define a new volume name that points at auth/TLS Secrets (for example signing-key secrets) and then mount that new volume in a sidecar.

Impact: users with template-write access can exfiltrate JWT signing private keys or other operator-managed secret material and forge credentials beyond intended secret-read boundaries.

 

^{Reviewed by Cursor Security Reviewer for commit b776dae. Configure here.}

[kop]

Leaving this one open — it's the same policy question as the sidecar reserved-volume thread (#discussion_r3586622811, awaiting @fstr's call). Name-based blocking can't fully close it: a template author can reach an operator-owned Secret via a differently-named volume's secretName, or via env.valueFrom.secretKeyRef / envFrom / a projected or CSI volume. If we decide template-write should be lower-privilege than Secret-read, I'll add a check rejecting user volumes whose secretName targets an operator-owned Secret to narrow this; otherwise we accept it as risk. Deferring to that decision.

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, and Cursor Security Agent left unresolved findings (TLS downgrade window and secret volume secretName bypass). Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[fstr]

Why shouldn't a sidecar be able to mount it?

[kop]

The block guards a specific privilege-escalation path, not sidecars in general. An engine-class template author needs only template-write access — not Kubernetes Secret-read RBAC — yet the operator renders the auth/TLS Secret volumes (auth-admin, auth-signing-<kid>, tls-engine) at pod level. Without this check, such an author could add a sidecar that mounts one of those volumes and read the JWT signing private keys or the admin password straight off disk — i.e. obtain credentials they can't otherwise kubectl get secret. That's the escalation being blocked.

Two honest caveats:

1. The block matches by volume name only, so it's structurally incomplete — a template author can declare a new pod-level volume pointing at the same Secret via secretName (and reach it other ways too: env.valueFrom.secretKeyRef, envFrom, projected/CSI volumes). This is exactly Cursor's open HIGH on operatorauthority.go. Name-matching alone can't fully close it.
2. If engine-class template authors in our model are effectively cluster admins (who could read those Secrets anyway), the restriction is friction for no real gain.

So the real question is: do we treat template-write as a lower privilege than Secret-read? If yes, I'd keep the block and additionally reject user volumes whose secretName targets an operator-owned Secret to narrow (not fully close) the bypass. If no (template authors are trusted), I'll drop the block and mark the Cursor findings accepted-risk. Which matches our threat model?

[gm42]

(AI-assisted review)

Review notes: PR #39 — auth/SSO, TLS, signing-key rotation (FB-896)

Overall this is a well-designed and carefully documented change. The rotation
state machine is thoughtfully put together: one step per reconcile keeps every
transition idempotent and crash-safe, the retain window is anchored at
confirmed post-promotion convergence rather than at the promotion decision,
generation-1 keeps its pre-rotation Certificate name so upgrades never force a
re-mint, and the packdb-side constraints (startup-only key reads, PKCS8
encoding, RotationPolicy: Never) were verified against engine source rather
than assumed. The doc comments throughout are excellent.

The notes below are mostly about closing the gap between the design and what
is exercised in practice.

1. Engine convergence lookup (worth addressing before merge)

The rotation gates rely on enginesConvergedOn (instance_auth.go:636) to
enumerate the instance's engines, and it currently does so with a label
selector on firebolt.io/instance. As far as I can tell, FireboltEngine CRs
don't carry that label: engines bind to an instance via spec.instanceRef,
there is no mutating webhook or controller write that stamps the label onto
the engine CR, and the examples / e2e helpers don't add it either. The
existing instance→engine watch mapping (engine_controller.go:1301) matches
on spec.instanceRef for the same reason.

With no engines matching the selector, the List comes back empty and the
"no engines is vacuously converged" branch returns true — so in a live
cluster the promote / retain-anchor / remove steps would advance without
actually waiting on engine convergence. That would quietly re-open the
sign-vs-validate window the gating was designed to close, and could allow an
old key's Secret to be removed while a slower engine still wants it for
validation.

The unit tests pass because the fixture builder (engineWithHash,
instance_auth_test.go:539) creates engines with the label already set, so
the assumption isn't exercised against a realistic CR.

Suggested fix is small: enumerate engines the same way the watch mapper does
(match on spec.instanceRef, or add a field index for it) instead of the
label selector, and adjust the fixtures accordingly.

2. TLA+ coverage

Two places where the formal side could catch up with the code:

• The rotation state machine (mint → promote → retain → remove, gated on
  cross-resource convergence) is the kind of protocol this repo usually
  models under formal/. The doc comments already state the key invariants
  in prose ("only one non-Active key outstanding", "safe to interrupt at any
  point"); capturing them in a spec would let TLC check them, and modeling
  the gate would also have required pinning down "the set of engines" —
  which is where item 1 hides.
• The Instance Ready roll-up now includes EngineTLSReady /
  GatewayTLSReady, but FireboltInstance.tla wasn't extended; the
  state-cover harness represents a TLS-disabled instance instead
  (instance_tla_state_test.go). Reasonable as a first step, but it leaves
  the TLS-enabled region of the roll-up outside the model. Also worth noting
  the TLC model checker required check is path-scoped and didn't run on
  this PR at all.

Neither has to block the merge, but a follow-up issue for the rotation spec
would keep the "a change in one belongs in both" convention intact.

3. E2E coverage

test/e2e/ is unchanged, so auth, TLS, and rotation currently ship with
unit / golden-file coverage only. The PR is upfront about this in the known
limitations, which is appreciated. Two considerations for a follow-up:

• The comment in ensureSigningKeys (instance_auth.go:278) says coverage
  for the apply-and-issue path "lives in e2e" — the cert-manager install in
  the e2e helpers does exist, but no e2e spec exercises the path yet, so the
  comment is a bit ahead of reality. Worth rewording or, better, making true.
• Since envtest deliberately can't cover the Certificate apply path (no
  cert-manager CRDs), e2e is the only harness that can observe it — and the
  convergence gating in particular really needs a real engine rolling to be
  meaningfully tested. A minimal spec (enable auth, trigger a rotation,
  assert promote waits for the engine) would cover both.

4. Smaller items

• Docs: docs/architecture.mdx isn't updated, while the PR adds new
  conditions, a new state machine, and cert-manager Certificate RBAC —
  the repo convention asks for the architecture doc to move in the same
  change. AGENTS.md could also mention the new spec.auth / spec.tls
  surface and reserved volumes.
• kubectl-firebolt: internal/infra/ wasn't touched; it compiles, but a
  quick pass to confirm the plugin's view of readiness conditions and the new
  CRD surface still lines up would satisfy the "verify against the plugin"
  rule for large changes.
• Rotation observability: if any engine stops reconciling, rotation
  waits indefinitely — which is the right fail-safe default, but nothing
  currently surfaces "rotation is waiting on engine X" (AuthReady stays
  True). A condition or event for a stalled rotation would help operators.
• PR description drift: the embedded summary still says the TLS
  conditions are not folded into top-level Instance Ready, but b776dae
  gates Ready on them — worth refreshing the description so reviewers reason
  from the current behavior.

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewer gm42 is already assigned; assigning fstr for continued review.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent has an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Stale comment

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}

[cursor]

Risk: high. Not approving: this auth/SSO/TLS/signing-key change exceeds the medium risk threshold, Cursor Security Agent left an unresolved HIGH finding (secret volume secretName bypass), and fstr has requested changes. Reviewers gm42 and fstr are already assigned.

  

_{Sent by Cursor Approval Agent: Infra Reviewer}