diff --git a/VERSION b/VERSION index 566871c..e831019 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.20 +0.10.21 diff --git a/internal/cli/client_status.go b/internal/cli/client_status.go index a5ef790..446618d 100644 --- a/internal/cli/client_status.go +++ b/internal/cli/client_status.go @@ -21,7 +21,7 @@ import ( ) func newClientStatusCmd() *cobra.Command { - var wait, seal bool + var wait, seal, iKnowTarget bool var timeout time.Duration var kubeconfigPath, contextOverride, nsOverride string cmd := &cobra.Command{ @@ -63,7 +63,7 @@ Exit codes with --seal: if cmd.Flags().Changed("timeout") && !wait && !seal { return &exitError{code: exitFailure, err: errors.New("--timeout has no effect without --wait or --seal")} } - for _, name := range []string{"kubeconfig", "context", "namespace"} { + for _, name := range []string{"kubeconfig", "context", "namespace", knowTargetFlag} { if cmd.Flags().Changed(name) && !seal { return &exitError{code: exitFailure, err: fmt.Errorf("--%s has no effect without --seal", name)} } @@ -71,7 +71,7 @@ Exit codes with --seal: if seal { return runSealCheck(cmd.Context(), printerFor(cmd), cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride}, - timeout) + timeout, iKnowTarget) } return runClientStatus(cmd.Context(), printerFor(cmd), wait, timeout) }, @@ -85,6 +85,10 @@ Exit codes with --seal: "with --seal: "+kubeconfigFlagUsage, "with --seal: "+contextFlagUsage) addNamespaceFlag(cmd, &nsOverride, "with --seal: "+namespaceFlagUsage) + // Registered directly (not addKnowTargetFlag) so the help carries the same + // "with --seal:" prefix as the other cluster-side flags above: it is inert + // without --seal, and the rejection loop enforces that. + cmd.Flags().BoolVar(&iKnowTarget, knowTargetFlag, false, "with --seal: "+knowTargetFlagUsage) return cmd } diff --git a/internal/cli/clusterguard.go b/internal/cli/clusterguard.go index eb98937..0502db3 100644 --- a/internal/cli/clusterguard.go +++ b/internal/cli/clusterguard.go @@ -2,8 +2,11 @@ package cli import ( "context" + "errors" "fmt" + "time" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/ui" @@ -12,78 +15,275 @@ import ( // clusterIDFromFn is a test seam over cluster.ClusterIDFrom. var clusterIDFromFn = cluster.ClusterIDFrom -// guardActiveClientCluster refuses a MUTATING operation when the cluster actually -// reached is not the one the active client lives on. +// targetVerifyTimeout bounds the backend lookup guardActiveClientCluster makes to +// NAME the target cluster. The lookup only makes the guard better-informed, so it +// must never make a mutating command slower than the mutation would be: past this, +// the guard falls back to the local anchor and the --no-input contract below. +const targetVerifyTimeout = 8 * time.Second + +// listAccountClientsFn resolves the signed-in account's clients for the target +// verifier (verifyTargetFromAPI). Production authenticates with the stored token +// and calls the backend; tests override it. An error means "couldn't ask the API" +// — offline, not signed in, or a backend failure — which the verifier treats as +// "couldn't confirm", never as "the account has no clients". +var listAccountClientsFn = func(ctx context.Context) ([]api.ProvisionedClient, error) { + client, _, err := authedClient() + if err != nil { + return nil, err + } + return client.ListClients(ctx) +} + +// guardActiveClientCluster refuses a MUTATING operation unless it can confirm WHICH +// cluster it is about to change — and prints that identity when it can. // -// WHY THIS EXISTS (backend#2863). Every command resolved its target cluster from the -// ambient kubeconfig + current-context, while binding only the NAMESPACE from the -// active client. So on a machine whose current-context pointed elsewhere — a laptop -// that also administers a managed cluster, which is the normal case for anyone who -// runs both — a mutating command acted on that other cluster: +// WHY THIS EXISTS (backend#2863, then backend#2983). Every command resolved its +// target cluster from the ambient kubeconfig + current-context, while binding only +// the NAMESPACE from the active client. So on a machine whose current-context +// pointed elsewhere — a laptop that also administers a managed cluster, which is the +// normal case for anyone who runs both — a mutating command acted on that other +// cluster: // // - `data ingest` staged a private dataset onto it // - `data delete` dropped a table and removed files from its shared PVC // - `resources set` rolled its jobs-manager with a new envelope // - `tracebloc delete` uninstalled a release of the same name // -// The namespace binding made this MORE likely, not less: it supplied a namespace that -// probably exists on the other cluster too, so discovery succeeded and nothing looked -// wrong. The hazard was already documented at internal/nodeboot/nodeboot.go, whose -// comment ends "preserving the default-context behavior" — that clause was the bug. +// backend#2863 added a local anchor to compare against, but left one path open: a +// machine with NO recorded anchor warned "the target couldn't be verified … +// Proceeding against https://127.0.0.1:8444" and then mutated anyway (backend#2983). +// The endpoint it named is a localhost port-forward — the same command shape reaches +// a different fleet depending on which tunnel is up on which port — so the operator's +// only signal about which environment was about to change was a port number, and +// --no-input (whose contract is "fail on missing required values") did not fail +// closed. A CI job could ingest into the wrong fleet and get a green exit. +// +// The fix is two-fold, matching the issue's directions: +// +// 1. VERIFY, don't merely record. Ask the API which client owns the cluster we +// actually reached and print THAT identity (name + namespace) — an identity +// that does not depend on local state, which is precisely what was missing in +// the field. verifyTargetFromAPI keys on the live kube-system UID, so the +// printed name is the backend's answer, not config's. +// 2. FAIL CLOSED when the target is unverifiable. If the API can't confirm the +// target and no recorded anchor matches it, refuse (exit non-zero, no write) +// unless the operator explicitly accepts the target with --i-know-the-target. +// A scripted (--no-input) caller has no human to ask, so this honours its +// contract; a human gets the same escape hatch, named in the message. // // The check is on IDENTITY, not on the context name. Pinning a context string would // break bring-your-own-cluster installs (EKS/AKS/OpenShift have no k3d context) and // would still pass if two kubeconfigs named the same context differently. The -// kube-system namespace UID is the same anchor the backend record uses, so local and -// remote agree by construction. +// kube-system namespace UID is the same anchor the backend record uses, so local, +// remote, and backend agree by construction. // // FAILURE MODES, deliberately asymmetric: // -// - mismatch -> REFUSE. Naming both ids and the way forward. -// - id unreadable -> REFUSE. We are about to write to a cluster we cannot -// identify. Every caller needs API access anyway, so this -// costs nothing legitimate. -// - no anchor recorded -> WARN and proceed. Configs written before this field -// exists must not be locked out of their own commands; -// `client create` records it and the warning names that. +// - id unreadable -> REFUSE. We are about to write to a cluster we cannot +// identify at all. --i-know-the-target overrides (nothing left to check). +// - anchor mismatch -> REFUSE. Affirmative evidence of the WRONG cluster; +// --i-know-the-target does NOT override a known-wrong target. +// - API-verified -> print the identity and proceed. +// - anchor matches -> proceed (positive local evidence; works offline). +// - unverifiable -> REFUSE unless --i-know-the-target (backend#2983). // // Read-only commands never call this: being wrong about which cluster you are // READING is a confusing answer, not a destructive act, and the target is already // printed by doctor / cluster info. -func guardActiveClientCluster(ctx context.Context, p *ui.Printer, t *clusterTarget) error { +func guardActiveClientCluster(ctx context.Context, p *ui.Printer, t *clusterTarget, ackTarget bool) error { if t == nil || t.Clientset == nil { return nil // nothing resolved (a test seam, or a command that mutates nothing) } - cfg, err := config.Load() - if err != nil { - // No readable config means no anchor to compare against — same case as an - // unrecorded anchor below, not a reason to block. - p.Warnf("Couldn't read the local config, so this machine's cluster couldn't be " + - "verified before changing anything. Proceeding.") - return nil - } - want := cfg.Current().ActiveClientClusterID - if want == "" { - p.Warnf("This machine hasn't recorded which cluster its secure environment runs on, "+ - "so the target couldn't be verified before changing anything. Run `tracebloc client "+ - "create` to record it. Proceeding against %s.", t.Resolved.ServerURL) - return nil - } + serverURL := t.Resolved.ServerURL + + // 1. Read the live cluster's own identity — the kube-system UID, the fingerprint + // the backend record and the local anchor both key on. Offline-readable; + // unreadable means we cannot name what we are about to write to. got, idErr := clusterIDFromFn(ctx, t.Clientset) if idErr != nil { + // A Ctrl-C (or parent deadline) during the read is an operator abort, not an + // unreadable cluster: exit quietly (130) rather than refuse-or-proceed on it. + if err := interrupted(ctx); err != nil { + return err + } + if ackTarget { + p.Warnf("Couldn't read this cluster's identity (%v) — proceeding anyway because "+ + "--i-know-the-target was set. Target: %s.", idErr, serverURL) + return nil + } return &exitError{code: exitLocalEnv, err: fmt.Errorf( "couldn't confirm which cluster this is before changing anything (%w).\n"+ " Refusing rather than writing to an unidentified cluster.\n"+ - " reached: %s", idErr, t.Resolved.ServerURL)} + " reached: %s\n"+ + " Fix your --context/--kubeconfig, or pass --i-know-the-target if you are certain.", + idErr, serverURL)} } - if got != want { + + // 2. Local anchor mismatch (backend#2863): the machine recorded which cluster its + // secure environment runs on, and this is a DIFFERENT one. Affirmative evidence + // of the wrong target — a hard refusal that --i-know-the-target does NOT + // override (the flag accepts an UNKNOWN target, never a known-wrong one). + want := recordedClusterAnchor() + if want != "" && got != want { return &exitError{code: exitLocalEnv, err: fmt.Errorf( "this is not the cluster your secure environment runs on — refusing to change anything.\n"+ " reached: %s (cluster %s, context %q)\n"+ " expected: cluster %s\n"+ " Your kubeconfig's current context points somewhere else. Either switch it, or pass\n"+ " --context/--kubeconfig for the cluster your secure environment runs on.", - t.Resolved.ServerURL, short(got), t.Resolved.Context, short(want))} + serverURL, short(got), t.Resolved.Context, short(want))} + } + + // 3. VERIFY THE IDENTITY FROM THE API (backend#2983, direction 1). Ask tracebloc + // which client owns the cluster we reached and print THAT — an identity that + // does not depend on local state (a missing local anchor is exactly what fell + // open in the field). Authoritative and self-checking: the printed name is the + // backend record keyed on the live cluster fingerprint, never config's. + c, reached, verr := verifyTargetFromAPI(ctx, got) + if verr != nil { + // Only a 426 (this CLI is too old for the server) reaches here — every other + // lookup failure is folded into reached=false below. A too-old CLI must + // HARD-STOP before mutating and before the offline anchor-match: it is exactly + // when we must not press on against a stale local anchor (Bugbot; learned rule + // "HTTP 426 must be a hard failure, not a warning"). --i-know-the-target does + // not override it — upgrading is the only way through, and the error says so. + return &exitError{code: exitFailure, err: verr} + } + // A Ctrl-C during the backend lookup cancels our context and surfaces as a plain + // lookup error, which verifyTargetFromAPI folds into reached=false. Left unchecked + // that would let the anchor-match below AUTHORIZE the mutation after the operator + // aborted (Bugbot), or refuse with a misleading "couldn't reach / run login" at + // exit 3 instead of a quiet 130. Catch the abort here — the inner verify timeout + // does not cancel THIS context, so a merely-slow backend still reads as reached=false. + if err := interrupted(ctx); err != nil { + return err + } + if c != nil { + p.Successf("Target verified with tracebloc: %s (namespace %s) — cluster %s.", + c.Name, c.Namespace, short(got)) + return nil + } + + // 3b. The API couldn't name it (offline / not signed in / no such record), but the + // machine's RECORDED anchor matches the live cluster: the operator ran + // `client create` here, so this is positive local evidence — proceed. This is + // NOT the backend#2983 fail-open, which was NO evidence at all (want == ""). + if want != "" && got == want { + p.Infof("Target matches this machine's recorded cluster (%s) — proceeding.", short(got)) + return nil + } + + // 4. UNVERIFIABLE: the API can't confirm the target and no recorded anchor matches + // it. This is the backend#2983 fail-open. Honour --no-input's contract — a + // scripted caller has no human to ask, so FAIL CLOSED — and require an explicit + // --i-know-the-target to proceed otherwise. + if ackTarget { + p.Warnf("Couldn't verify which cluster this is — proceeding anyway because "+ + "--i-know-the-target was set. Target: %s (cluster %s).", serverURL, short(got)) + return nil + } + // The refusal must not overstate what's known. Two distinct situations, and + // neither may assert an ABSENCE: + // - reached=false: we couldn't ask the API at all — say exactly that. + // - reached=true, no anchored match: the API answered but nothing in the account + // is anchored to this cluster's UID. This is NOT "no client here" — discovery + // already found a client release on this cluster to get us this far, and a + // legacy client's ClusterID is empty so it can never match by UID (Bugbot). + // So the honest framing is "not LINKED to your account records", and + // `client create` is the right fix precisely because it ADOPTS the on-cluster + // client and records the anchor. + if reached { + return &exitError{code: exitLocalEnv, err: fmt.Errorf( + "couldn't verify which cluster this is before changing anything — the tracebloc client on this "+ + "cluster isn't linked to any client in your account (it may be newly installed, or created "+ + "before cluster anchoring), and this machine hasn't recorded one.\n"+ + " reached: %s (cluster %s)\n"+ + " Refusing rather than writing to an unverified cluster.\n"+ + " Run `tracebloc client create` — it adopts the client already on this cluster and records it — "+ + "or fix your --context/--kubeconfig, or\n"+ + " re-run with --i-know-the-target if you are certain this is the right cluster.", + serverURL, short(got))} + } + return &exitError{code: exitLocalEnv, err: fmt.Errorf( + "couldn't verify which cluster this is before changing anything — tracebloc couldn't be reached to "+ + "confirm the target, and this machine hasn't recorded one.\n"+ + " reached: %s (cluster %s)\n"+ + " Refusing rather than writing to an unverified cluster.\n"+ + " Check you're signed in (`tracebloc login`) and pointed at the right --context/--kubeconfig, or\n"+ + " re-run with --i-know-the-target if you are certain this is the right cluster.", + serverURL, short(got))} +} + +// recordedClusterAnchor returns this machine's recorded cluster anchor (the +// active client's kube-system UID), or "" when there is no readable config or no +// anchor. Best-effort by contract: a config it cannot read is "no anchor", which +// routes to the API verification / fail-closed path rather than a hard error. +func recordedClusterAnchor() string { + cfg, err := config.Load() + if err != nil { + return "" + } + return cfg.Current().ActiveClientClusterID +} + +// verifyTargetFromAPI asks the backend which client owns the cluster identified by +// clusterID (the kube-system UID) and returns that record. The identity it returns +// is the API's, not local config's — which is the whole point (backend#2983): the +// field failure was a MISSING local anchor, so a check that read local state could +// never have caught it. +// +// It reports THREE outcomes, because collapsing "couldn't reach the API" into "the +// API says there is no such client" is the #515 trap the rest of the codebase is +// careful about (DiscoverInClusterClientID's three-valued return, clientSurvey.looked): +// telling an operator "tracebloc has no client for this cluster — run client create" +// when we simply could not ask is a false absence, and `client create` on an +// UNCONFIRMED cluster is the adopt-vs-mint hazard. +// +// - (client, true, nil) — the API answered and a client is anchored here: verified. +// - (nil, true, nil) — the API answered but NO account client is anchored to this +// cluster's UID. NOT a proof of absence: discovery already found a client release +// on the cluster, and a legacy client's ClusterID is empty so it can never match by +// UID. The caller frames this as "not linked to your account" and points at +// `client create`, which adopts the on-cluster client and records the anchor. +// - (nil, false, nil) — the API could not be reached / not signed in / a transient +// backend error: we could NOT ask. The caller must not assert absence. +// - (nil, false, err) — the API returned 426 Upgrade Required. This alone +// propagates as an error: a CLI too old for the server must HARD-STOP, never be +// swallowed and then press on against a local anchor (learned rule / Bugbot). +func verifyTargetFromAPI(ctx context.Context, clusterID string) (client *api.ProvisionedClient, reached bool, err error) { + if clusterID == "" { + return nil, false, nil + } + ctx, cancel := context.WithTimeout(ctx, targetVerifyTimeout) + defer cancel() + clients, lerr := listAccountClientsFn(ctx) + if lerr != nil { + var ue *api.UpgradeRequiredError + if errors.As(lerr, &ue) { + return nil, false, lerr // 426 → hard-stop; the caller exits with the upgrade message + } + return nil, false, nil // offline / not signed in / transient → couldn't ask + } + // The API answered (reached=true). anchoredClient is the same kube-system-UID → + // client match `client create` uses to decide adopt-vs-mint; reuse it so the two + // paths can't drift. A nil result here is a CONFIRMED absence, not a couldn't-ask. + return anchoredClient(clients, clusterID), true, nil +} + +// interrupted returns the quiet Ctrl-C exit (130) when the command's context was +// cancelled during a blocking guard step — an operator abort (or a parent deadline), +// NOT an unreadable/unreachable cluster. It checks the context itself, so it is not +// fooled by verifyTargetFromAPI's inner timeout (which cancels only its own derived +// context, never this one). Returns nil while the context is still live, so callers +// fall through to their normal refuse/proceed logic. +// +// The exitError carries NO inner err on purpose: exitInterrupted is a SILENT exit +// (IsSilentError keys on err==nil), so main() prints nothing — same as every other +// interrupt path (auth.go, seal.go, client_status.go). Attaching ctx.Err() would +// print a bare "Error: context canceled" on Ctrl-C. +func interrupted(ctx context.Context) error { + if ctx.Err() != nil { + return &exitError{code: exitInterrupted} } return nil } diff --git a/internal/cli/clusterguard_test.go b/internal/cli/clusterguard_test.go index 5fa046f..1ff94d6 100644 --- a/internal/cli/clusterguard_test.go +++ b/internal/cli/clusterguard_test.go @@ -17,6 +17,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" @@ -89,9 +90,11 @@ func TestResolveClusterTarget_Mutating_WrongCluster_Refuses(t *testing.T) { withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("BBBBBBBB-9999-8888-7777-666666666666"))) withClusterID(t, "BBBBBBBB-9999-8888-7777-666666666666", nil) + withAccountClients(t, nil, errors.New("offline (test): no backend")) + var out bytes.Buffer _, err := resolveClusterTarget(context.Background(), ui.New(&out), - cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true) + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false) if err == nil { t.Fatal("a mutating command must refuse a cluster that is not the recorded one") } @@ -122,7 +125,7 @@ func TestResolveClusterTarget_ReadOnly_WrongCluster_Proceeds(t *testing.T) { var out bytes.Buffer if _, err := resolveClusterTarget(context.Background(), ui.New(&out), - cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, false); err != nil { + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, false, false); err != nil { t.Fatalf("a read-only command must not be gated on cluster identity: %v", err) } } @@ -135,10 +138,13 @@ func TestResolveClusterTarget_Mutating_RightCluster_Proceeds(t *testing.T) { withAnchor(t, "SAME-CLUSTER-UID") withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("SAME-CLUSTER-UID"))) withClusterID(t, "SAME-CLUSTER-UID", nil) + // API offline: the recorded anchor matching the live cluster is the positive + // local evidence that must let the command through even when the API can't name it. + withAccountClients(t, nil, errors.New("offline (test): no backend")) var out bytes.Buffer if _, err := resolveClusterTarget(context.Background(), ui.New(&out), - cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true); err != nil { + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false); err != nil { t.Fatalf("the recorded cluster must be usable: %v", err) } } @@ -154,7 +160,7 @@ func TestResolveClusterTarget_Mutating_UnreadableID_Refuses(t *testing.T) { var out bytes.Buffer _, err := resolveClusterTarget(context.Background(), ui.New(&out), - cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true) + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false) if err == nil { t.Fatal("an unidentifiable cluster must not be written to") } @@ -166,25 +172,328 @@ func TestResolveClusterTarget_Mutating_UnreadableID_Refuses(t *testing.T) { } } -// NO anchor recorded → warn and proceed. Configs written before this field -// existed must not be locked out of their own commands, and the warning has to -// name the way to fix it or it is just noise. -func TestResolveClusterTarget_Mutating_NoAnchor_WarnsAndProceeds(t *testing.T) { +// The security asymmetry, pinned as a mutation-proof (Lukas' review): the whole +// safety story rests on "--i-know-the-target overrides an UNKNOWN target, never a +// known-WRONG one." So a wrong-cluster mismatch must STILL refuse even with the ack +// flag set. If a future edit "for symmetry" made the mismatch branch consult +// ackTarget, this reddens — the flag would otherwise become a wrong-cluster bypass. +func TestResolveClusterTarget_Mutating_WrongCluster_AckDoesNotOverride(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "AAAAAAAA-1111-2222-3333-444444444444") + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("BBBBBBBB-9999-8888-7777-666666666666"))) + withClusterID(t, "BBBBBBBB-9999-8888-7777-666666666666", nil) + withAccountClients(t, nil, errors.New("offline (test): no backend")) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, true /* --i-know-the-target */) + if err == nil { + t.Fatal("--i-know-the-target must NOT override a KNOWN-WRONG cluster — only an unknown one") + } + if !strings.Contains(err.Error(), "not the cluster your secure environment runs on") { + t.Errorf("the mismatch refusal must stand even with the ack flag; got: %v", err) + } +} + +// The unreadable-ID + ack branch (Lukas' review #2): with --i-know-the-target, a +// cluster whose identity can't even be read proceeds with a warning rather than +// refusing — the ack overrides the one unverifiable case where there is nothing +// left to check. +func TestResolveClusterTarget_Mutating_UnreadableID_AckProceeds(t *testing.T) { t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"))) - withClusterID(t, "WHATEVER", nil) + withClusterID(t, "", errors.New("namespaces \"kube-system\" is forbidden")) var out bytes.Buffer if _, err := resolveClusterTarget(context.Background(), ui.New(&out), - cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true); err != nil { - t.Fatalf("an unrecorded anchor must not block: %v", err) + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, true /* --i-know-the-target */); err != nil { + t.Fatalf("--i-know-the-target must let an unreadable-identity cluster through: %v", err) + } + if got := out.String(); !strings.Contains(got, knowTargetFlag) { + t.Errorf("proceeding on an unreadable identity must say why (named --%s); got:\n%s", knowTargetFlag, got) + } +} + +// A 426 (this CLI is too old for the server) must HARD-STOP, never be swallowed as +// "couldn't confirm" and then wave the mutation through on a matching local anchor +// (Bugbot / learned rule). The anchor here MATCHES the live cluster, so without the +// hard-stop the guard would proceed via the offline anchor-match path; it must +// instead refuse and point at `tracebloc upgrade`, and --i-know-the-target must not +// override it (upgrading is the only way through). +func TestResolveClusterTarget_Mutating_UpgradeRequired_HardStops(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "MATCHING-UID") // would otherwise proceed via anchor-match (3b) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("MATCHING-UID"))) + withClusterID(t, "MATCHING-UID", nil) + withAccountClients(t, nil, &api.UpgradeRequiredError{MinVersion: "0.11.0"}) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, true /* even with ack */) + if err == nil { + t.Fatal("a 426 must hard-stop the mutation, not be swallowed and proceed on the local anchor") + } + var ue *api.UpgradeRequiredError + if !errors.As(err, &ue) { + t.Errorf("the 426 must propagate as an UpgradeRequiredError so the exit says 'upgrade'; got: %v", err) + } + if !strings.Contains(err.Error(), "tracebloc upgrade") { + t.Errorf("the refusal must point at the upgrade, not `client create`; got: %v", err) + } +} + +// A Ctrl-C DURING the backend verify (its context cancelled) must be a quiet +// interrupt (exit 130), NOT swallowed into reached=false — which, on a MATCHING +// recorded anchor, would authorize the mutation the operator just aborted (Bugbot), +// or refuse with a misleading "couldn't reach / run login" at exit 3. +func TestResolveClusterTarget_Mutating_CtrlCDuringVerify_Interrupts(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "MATCHING-UID") // matches → without the check, 3b would PROCEED + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("MATCHING-UID"))) + withClusterID(t, "MATCHING-UID", nil) + // The lookup returns a cancellation error, mirroring an aborted in-flight request. + withAccountClients(t, nil, context.Canceled) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // the operator hit Ctrl-C + + var out bytes.Buffer + _, err := resolveClusterTarget(ctx, ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false) + if err == nil { + t.Fatal("a Ctrl-C during verify must abort, not authorize the mutation on a matching anchor") + } + if got := ExitCodeFromError(err); got != exitInterrupted { + t.Errorf("exit code = %d, want %d (quiet Ctrl-C), not a refusal/proceed; err: %v", got, exitInterrupted, err) + } + // exitInterrupted is a SILENT exit — main() must print nothing (no bare + // "Error: context canceled"), same as every other interrupt path. + if !IsSilentError(err) { + t.Errorf("a Ctrl-C exit must be silent (no inner err), else main() prints 'Error: context canceled'; err: %v", err) + } + if strings.Contains(out.String(), "matches this machine's recorded cluster") { + t.Errorf("the anchor-match proceed must NOT fire after an abort; got:\n%s", out.String()) + } +} + +// withAccountClients stubs the backend clients lookup the target verifier +// (verifyTargetFromAPI) makes, so a guard test can drive "the API names this +// cluster" / "the API can't confirm" without standing up a server. An error models +// offline / not-signed-in / a backend failure. +func withAccountClients(t *testing.T, clients []api.ProvisionedClient, err error) { + t.Helper() + orig := listAccountClientsFn + t.Cleanup(func() { listAccountClientsFn = orig }) + listAccountClientsFn = func(context.Context) ([]api.ProvisionedClient, error) { + return clients, err + } +} + +// withActiveClientName records a display name (but NO cluster anchor) for the +// active client, so a test can prove the guard's printed identity comes from the +// API and not from this local cache. +func withActiveClientName(t *testing.T, name string) { + t.Helper() + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + if cfg.CurrentEnv == "" { + cfg.CurrentEnv = "test" + } + cfg.Current().ActiveClientName = name + if err := cfg.Save(); err != nil { + t.Fatal(err) + } +} + +// backend#2983 direction 1: with NO local anchor — the exact field condition that +// fell open — the guard VERIFIES the target against the API and prints the fleet's +// identity (name + namespace), an identity that comes from the API record and not +// from local state. Here the local cache carries a DECOY name; the printed identity +// must be the API's, and the command proceeds. +func TestResolveClusterTarget_Mutating_VerifiesIdentityFromAPI(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withActiveClientName(t, "stale-local-decoy") // no ActiveClientClusterID → no anchor + withClusterSeams(t, fake.NewSimpleClientset(jmDep("hasan-prod"), kubeSystem("KUBE-UID-PROD"))) + withClusterID(t, "KUBE-UID-PROD", nil) + withAccountClients(t, []api.ProvisionedClient{ + {ID: 9, Name: "other-box", Namespace: "other-ns", ClusterID: "SOME-OTHER-UID"}, + {ID: 7, Name: "hasan-prod", Namespace: "tracebloc-templates-prod", ClusterID: "KUBE-UID-PROD"}, + }, nil) + + var out bytes.Buffer + if _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false); err != nil { + t.Fatalf("an API-verified target must proceed: %v", err) } got := out.String() - if !strings.Contains(got, "hasn't recorded which cluster") { - t.Errorf("an unverified mutation must say so; got:\n%s", got) + for _, want := range []string{"hasan-prod", "tracebloc-templates-prod"} { + if !strings.Contains(got, want) { + t.Errorf("the printed identity must be the API's %q; got:\n%s", want, got) + } + } + // The name came from the API record, not the local cache — the whole point of + // backend#2983 direction 1 (local state is what was missing in the field). + if strings.Contains(got, "stale-local-decoy") { + t.Errorf("the identity must come from the API, not the local cache; got:\n%s", got) + } +} + +// backend#2983 direction 2, the API-UNREACHABLE case: no anchor, and the API can't +// be reached (offline / not signed in) — a scripted --no-input caller has no human +// to ask, so this fails closed rather than mutating a cluster identified only by a +// localhost port. And it must NOT assert an absence it never confirmed: a +// couldn't-reach refusal names sign-in / context, never `client create` on an +// unconfirmed cluster (the #515 adopt-vs-mint trap; Bugbot). +func TestResolveClusterTarget_Mutating_Unverifiable_Unreachable_FailsClosed(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("KUBE-UID-X"))) + withClusterID(t, "KUBE-UID-X", nil) + withAccountClients(t, nil, errors.New("dial tcp: connect: connection refused")) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false /* ack: as under --no-input */) + if err == nil { + t.Fatal("an unverifiable target must NOT proceed without --i-know-the-target — this is the backend#2983 fail-open") + } + if got := ExitCodeFromError(err); got != exitLocalEnv { + t.Errorf("exit code = %d, want %d (a local-environment problem)", got, exitLocalEnv) + } + msg := err.Error() + for _, want := range []string{"couldn't verify which cluster this is", "couldn't be reached", knowTargetFlag} { + if !strings.Contains(msg, want) { + t.Errorf("the refusal must name %q so it isn't a dead end; got:\n%s", want, msg) + } + } + // Must NOT claim a confirmed absence we never confirmed, nor push `client create` + // on an unconfirmed cluster (the #515 trap the fix closes). + for _, forbidden := range []string{"has no client registered", "client create"} { + if strings.Contains(msg, forbidden) { + t.Errorf("a couldn't-REACH refusal must not assert absence / recommend %q; got:\n%s", forbidden, msg) + } + } +} + +// The other half: the API WAS reached and answered, but no account client is +// anchored to this cluster's UID (here every record is anchored elsewhere; the +// legacy empty-ClusterID record is the same shape). This is NOT a confirmed absence +// — discovery already found a client release on the cluster — so the refusal must +// say the on-cluster client "isn't linked to any client in your account", NEVER +// "tracebloc has no client registered" (Bugbot). `client create` is the right fix +// because it adopts the on-cluster client and records the anchor. +func TestResolveClusterTarget_Mutating_ReachedButUnmatched_FailsClosed(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("KUBE-UID-X"))) + withClusterID(t, "KUBE-UID-X", nil) + // API answered, but no client is anchored to KUBE-UID-X (one anchored elsewhere, + // one legacy with an empty ClusterID — neither can match by UID). + withAccountClients(t, []api.ProvisionedClient{ + {ID: 3, Name: "someone-else", Namespace: "other", ClusterID: "A-DIFFERENT-UID"}, + {ID: 4, Name: "legacy-box", Namespace: "legacy", ClusterID: ""}, + }, nil) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, false) + if err == nil { + t.Fatal("a cluster whose client isn't linked to the account, with no anchor, must fail closed") + } + if got := ExitCodeFromError(err); got != exitLocalEnv { + t.Errorf("exit code = %d, want %d", got, exitLocalEnv) + } + msg := err.Error() + for _, want := range []string{"isn't linked to any client in your account", "client create", knowTargetFlag} { + if !strings.Contains(msg, want) { + t.Errorf("a reached-but-unmatched refusal should name %q; got:\n%s", want, msg) + } + } + // Must NOT assert an absence: discovery already found a client release here, and a + // legacy empty-ClusterID client can never match by UID (Bugbot). + if strings.Contains(msg, "has no client registered") { + t.Errorf("must not assert absence when a client release IS on the cluster; got:\n%s", msg) + } +} + +// The escape hatch: --i-know-the-target lets an operator through an unverifiable +// target on purpose, with a warning naming what little is known. Without this the +// fail-closed above would be a wall for the legitimate air-gapped / not-yet- +// registered case. +func TestResolveClusterTarget_Mutating_Unverifiable_AckProceeds(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("KUBE-UID-X"))) + withClusterID(t, "KUBE-UID-X", nil) + withAccountClients(t, nil, errors.New("dial tcp: connect: connection refused")) + + var out bytes.Buffer + if _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true, true /* --i-know-the-target */); err != nil { + t.Fatalf("--i-know-the-target must let an unverifiable target through: %v", err) + } + if got := out.String(); !strings.Contains(got, knowTargetFlag) { + t.Errorf("proceeding on an unverified target must say why (it named --%s); got:\n%s", knowTargetFlag, got) + } +} + +// The backend#2983 scenario end-to-end at the command boundary: `data ingest` +// under --no-input (AckTarget=false), against a cluster the API can't confirm and +// with no recorded anchor, must FAIL CLOSED before staging anything — not warn and +// write, which is what a CI job hit in the field. connectIngestTarget is the money +// path's pre-flight; the guard fires inside it, before the PVC discovery and the +// destination-table read, so nothing is touched by the time it refuses. +func TestConnectIngestTarget_Unverifiable_NoInput_FailsClosed(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + // A reachable cluster (kube-system UID readable), hosting a client, but the API + // can't say which fleet it is and this machine never recorded one. + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("KUBE-UID-CI"))) + withAccountClients(t, nil, errors.New("dial tcp: connect: connection refused")) + + var out bytes.Buffer + a := &runDataIngestArgs{Printer: ui.New(&out), AckTarget: false} // --no-input passes no ack + target, _, cancelled, err := connectIngestTarget(context.Background(), a) + if err == nil { + t.Fatal("data ingest --no-input against an unverifiable target must fail closed, not stage") + } + if target != nil || cancelled { + t.Errorf("nothing may be handed back on a fail-closed refusal (target=%v cancelled=%v)", target, cancelled) + } + if got := ExitCodeFromError(err); got != exitLocalEnv { + t.Errorf("exit code = %d, want %d — a CI job must get a non-zero exit, not a green run", got, exitLocalEnv) + } + if !strings.Contains(err.Error(), "couldn't verify which cluster this is") { + t.Errorf("the refusal must say the target couldn't be verified; got: %v", err) + } +} + +// The destructive twin the issue also called out: `data delete` drops a MySQL +// table and removes files on the shared PVC. An unverifiable target must refuse +// BEFORE the dataset list is even read — the teardown never gets a chance to run. +func TestRunDataDelete_Unverifiable_FailsClosedBeforeTeardown(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("KUBE-UID-D"))) + withAccountClients(t, nil, errors.New("dial tcp: connect: connection refused")) + // If anything past the guard runs, the delete is proceeding — fail loudly. + origList := listDatasetsFn + t.Cleanup(func() { listDatasetsFn = origList }) + listDatasetsFn = func(context.Context, kubernetes.Interface, *rest.Config, string) ([]string, error) { + t.Fatal("the delete reached the dataset list — the guard did not fail closed before teardown") + return nil, nil + } + + var out bytes.Buffer + // Yes:true is the scriptable delete; the guard must refuse regardless. + err := runDataDelete(context.Background(), runDataDeleteArgs{ + Table: "probe", Printer: ui.New(&out), Yes: true, AckTarget: false, + }) + if err == nil { + t.Fatal("data delete against an unverifiable target must refuse, not drop the table") + } + if got := ExitCodeFromError(err); got != exitLocalEnv { + t.Errorf("exit code = %d, want %d", got, exitLocalEnv) } - if !strings.Contains(got, "client create") { - t.Errorf("the warning must name how to record it; got:\n%s", got) + if !strings.Contains(err.Error(), "couldn't verify which cluster this is") { + t.Errorf("the refusal must say the target couldn't be verified; got: %v", err) } } @@ -358,8 +667,8 @@ func TestEveryClusterCallSiteDeclaresMutationIntent(t *testing.T) { if !ok || (id.Name != "resolveClusterTarget" && id.Name != "resolveClusterTargetFn") { return true } - if len(call.Args) != 7 { - t.Errorf("%s: resolveClusterTarget called with %d args, want 7 — the"+ + if len(call.Args) != 8 { + t.Errorf("%s: resolveClusterTarget called with %d args, want 8 — the"+ " mutates parameter is what forces the decision", f, len(call.Args)) return true } diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index 640525c..e50ab84 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -99,7 +99,11 @@ type clusterTarget struct { // the choice must be impossible to omit rather than an opt-in helper a new command // forgets to call. `true` runs guardActiveClientCluster before the target is handed // back; `false` is a read. -func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC, leadRedirect, mutates bool) (*clusterTarget, error) { +// +// ackTarget carries the mutating command's --i-know-the-target flag through to the +// guard: it lets an operator proceed against a cluster whose identity can't be +// verified (backend#2983). It is only read when mutates is true; reads pass false. +func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC, leadRedirect, mutates, ackTarget bool) (*clusterTarget, error) { resolved, err := loadClusterFn(opts) if err != nil { return nil, &exitError{code: exitLocalEnv, err: fmt.Errorf("loading kubeconfig: %w", err)} @@ -135,7 +139,7 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // back, so a mutating command cannot have touched anything by the time it is told // this is the wrong cluster. if mutates { - if err := guardActiveClientCluster(ctx, p, t); err != nil { + if err := guardActiveClientCluster(ctx, p, t, ackTarget); err != nil { return nil, err } } diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index daf67d6..adace5b 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -42,7 +42,7 @@ func withClusterSeams(t *testing.T, cs kubernetes.Interface) { func TestResolveClusterTarget_NoClient_InstallerMessageExit4(t *testing.T) { withClusterSeams(t, fake.NewSimpleClientset()) // empty cluster _, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false) + cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false, false) if err == nil { t.Fatal("expected an error when the cluster hosts no client") } @@ -68,7 +68,7 @@ func TestResolveClusterTarget_NoClient_InstallerMessageExit4(t *testing.T) { func TestResolveClusterTarget_MultipleClients_PickOneExit4(t *testing.T) { withClusterSeams(t, fake.NewSimpleClientset(jmDep("alpha"), jmDep("beta"))) _, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false) + cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false, false) if err == nil { t.Fatal("expected an error when multiple clients are present") } @@ -338,7 +338,7 @@ func TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting(t *testing.T) binding := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "stale-ns"} target, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false, false) + cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false, false, false) if err == nil { t.Fatal("a binding miss must still fail — this changes the message, not the target") } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 27ae219..dc0bd31 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -33,6 +33,9 @@ type runDataDeleteArgs struct { // --output-json mode. Same contract as data list / data ingest. OutputJSON bool JSONOut io.Writer + // AckTarget carries --i-know-the-target: proceed even when the target + // cluster's identity can't be verified with tracebloc (backend#2983). + AckTarget bool } // newDataDeleteCmd implements `tracebloc data delete