Argo: keep the cross-cluster guard intact on failed-connect rollback + free the port-forward on auth failure

Two defects in the Argo credential model surfaced by review:

- A failed connect attempt with a fresh token stamped tokenContext to the current
  cluster before the probe, and the rollback re-pointed the manager with a plain
  SetConfig that leaves tokenContext untouched — so the restored (older)
  auto-discovery token was left marked valid for the wrong cluster in memory,
  defeating the cross-cluster guard until restart. Rollback now restores the
  captured prior binding via RestoreConfig on both failure paths (probe + persist).

- Out-of-cluster discovery installs a port-forward to a reachable argocd-server
  before verifying the token; a token failure returned without tearing it down,
  leaving a live localhost tunnel while the manager reports disconnected. Probe
  now drops the forward on auth failure (unless a config change already owns
  cleanup).

Author: nadaverell

internal/argocd/manager.go

@@ -148,6 +148,12 @@ func SetConfig(url, token string, insecureTLS bool, tokenIsFresh bool) {
 	defaultManager.SetConfig(url, token, insecureTLS, tokenIsFresh)
 }
 
+// RestoreConfig rolls the default manager back to a captured state, including
+// the token's context binding.
+func RestoreConfig(url, token string, insecureTLS bool, tokenContext string) {
+	defaultManager.RestoreConfig(url, token, insecureTLS, tokenContext)
+}
+
 // IsConfigured reports whether the default manager has connection settings.
 func IsConfigured() bool { return defaultManager.IsConfigured() }
 
@@ -219,6 +225,28 @@ func (m *Manager) SetConfig(url, token string, insecureTLS bool, tokenIsFresh bo
 	}
 }
 
+// RestoreConfig rolls the manager back to a previously-captured state, INCLUDING
+// the token's context binding. It exists for the connect-rollback path: the
+// candidate SetConfig may have re-stamped tokenContext for a fresh token, and a
+// plain SetConfig(fresh=false) rollback leaves that stamp in place — which would
+// mark the restored (older) token as valid for the wrong cluster and defeat the
+// auto-discovery cross-cluster guard. Restoring tokenContext explicitly keeps the
+// guard intact.
+func (m *Manager) RestoreConfig(url, token string, insecureTLS bool, tokenContext string) {
+	m.mu.Lock()
+	m.seeded = true
+	m.manualURL = strings.TrimRight(strings.TrimSpace(url), "/")
+	m.token = token
+	m.insecureTLS = insecureTLS
+	m.tokenContext = tokenContext
+	m.generation++
+	fwd := m.dropConnectionLocked()
+	m.mu.Unlock()
+	if fwd != nil {
+		fwd.stop()
+	}
+}
+
 // IsConfigured reports whether the Argo CD integration has connection settings
 // (an explicit URL or a token) — i.e. the user has set it up, even if a live
 // probe hasn't landed yet. Used to offer the deep-diff capability immediately
@@ -366,6 +394,20 @@ func (m *Manager) Probe(ctx context.Context) error {
 		return err
 	}
 	if err := m.verifyAuth(ctx, url, snap); err != nil {
+		// resolve may have installed a fresh port-forward to a reachable
+		// argocd-server; a token failure means we won't use that endpoint, so
+		// tear the tunnel down rather than leave a disconnected manager holding a
+		// live forward. Skip when a config change already superseded this probe —
+		// that SetConfig/Reset owns the cleanup.
+		m.mu.Lock()
+		var fwd *activeForward
+		if !m.staleLocked(snap) {
+			fwd = m.dropConnectionLocked()
+		}
+		m.mu.Unlock()
+		if fwd != nil {
+			fwd.stop()
+		}
 		return err
 	}
 

internal/argocd/manager_test.go

@@ -112,6 +112,43 @@ func TestSetConfigPreservesTokenContext(t *testing.T) {
 	}
 }
 
+// TestRestoreConfigRestoresTokenContextBinding pins the connect-rollback guard:
+// trying a fresh token in a new cluster re-stamps tokenContext to that cluster
+// BEFORE the probe. When the probe fails, the rollback must put the PRIOR binding
+// back onto the restored (older) token — otherwise that token would be left
+// marked valid for the wrong cluster in memory, defeating the cross-cluster guard
+// until restart. A plain SetConfig(fresh=false) rollback would leave the new
+// stamp; RestoreConfig restores the binding explicitly.
+func TestRestoreConfigRestoresTokenContextBinding(t *testing.T) {
+	ctx := "cluster-a"
+	m := newTestManager(config.Config{})
+	m.contextName = func() string { return ctx }
+
+	m.SetConfig("", "token-a", false, true) // fresh auto-discovery token on cluster-a
+	if m.tokenContext != "cluster-a" {
+		t.Fatalf("tokenContext = %q, want cluster-a", m.tokenContext)
+	}
+
+	// Operator switches to cluster-b and tries to connect a fresh token there; the
+	// candidate SetConfig(fresh=true) stamps tokenContext=cluster-b before the probe.
+	ctx = "cluster-b"
+	m.SetConfig("", "token-b", false, true)
+	if m.tokenContext != "cluster-b" {
+		t.Fatalf("candidate tokenContext = %q, want cluster-b", m.tokenContext)
+	}
+
+	// The probe fails, so the handler rolls back to the prior token + binding.
+	m.RestoreConfig("", "token-a", false, "cluster-a")
+	if m.tokenContext != "cluster-a" {
+		t.Fatalf("tokenContext after rollback = %q, want cluster-a restored", m.tokenContext)
+	}
+	// Still in cluster-b: the restored token is bound to cluster-a, so the guard
+	// must fire rather than send token-a to cluster-b's argocd-server.
+	if err := m.Probe(context.Background()); !errors.Is(err, errTokenContextMismatch) {
+		t.Fatalf("Probe after rollback = %v, want errTokenContextMismatch (guard intact)", err)
+	}
+}
+
 // TestIsConfigured pins that IsConfigured reflects whether the integration has
 // connection settings (explicit URL or token), independent of any live probe.
 func TestIsConfigured(t *testing.T) {

internal/server/argocd_integration.go

@@ -99,11 +99,18 @@ func (s *Server) handleApplyArgoCDConfig(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
+	// Capture the live token→context binding before the candidate SetConfig
+	// re-stamps it for a fresh token. If the probe fails we must restore THIS
+	// binding, not just the URL/token: a fresh-token SetConfig stamps
+	// tokenContext to the current cluster, and a plain re-point rollback would
+	// leave that stamp on the restored (older) token — marking it valid for the
+	// wrong cluster and defeating the auto-discovery cross-cluster guard.
+	prevTokenContext := argocd.TokenContext()
 	argocd.SetConfig(rawURL, token, body.ArgoCDInsecureTLS, suppliesNewToken)
 	ctx, cancel := context.WithTimeout(r.Context(), 12*time.Second)
 	defer cancel()
 	if err := argocd.Probe(ctx); err != nil {
-		argocd.SetConfig(prev.ArgoCDURL, prev.ArgoCDToken, prev.ArgoCDInsecureTLS, false)
+		argocd.RestoreConfig(prev.ArgoCDURL, prev.ArgoCDToken, prev.ArgoCDInsecureTLS, prevTokenContext)
 		// The upstream error can embed the raw response body (proxy headers, a
 		// render error with Secret data). Log it server-side; return only the
 		// mapped guidance so nothing from Argo's body reaches the browser.
@@ -132,8 +139,9 @@ func (s *Server) handleApplyArgoCDConfig(w http.ResponseWriter, r *http.Request)
 			}
 		}
 	}); err != nil {
-		// The running client must agree with the on-disk config; roll it back.
-		argocd.SetConfig(prev.ArgoCDURL, prev.ArgoCDToken, prev.ArgoCDInsecureTLS, false)
+		// The running client must agree with the on-disk config; roll it back —
+		// including the token's context binding (see prevTokenContext above).
+		argocd.RestoreConfig(prev.ArgoCDURL, prev.ArgoCDToken, prev.ArgoCDInsecureTLS, prevTokenContext)
 		log.Printf("[argocd] Failed to persist Argo CD config: %v", err)
 		s.writeError(w, http.StatusInternalServerError, err.Error())
 		return