🤖 fix: provider and MCP creates adopt only by a secret nonce from the cleanup record

docs/how-to/manage-ai-providers.md

@@ -134,7 +134,11 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re
 
 ### Cleanup records
 
-Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-<object UID>` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider.
+Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-<object UID>` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider.
+
+The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. Namespace users cannot read the record (see the access rule below), so they cannot give a provider that display name before the create. After the create commits, Coder shows the nonce to users who can read the provider, until the operator applies the spec. Until the operator learns the provider ID, normally at its next read a few seconds later, such a user could delete the provider and create another one of that name with that display name. Such a user can delete the provider anyway. Until the operator knows the provider ID, every create sends the same nonce, so a create that commits late is still recognized. After it knows the ID, it never takes another provider by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:<object UID>`, which anyone who can read the object sees. The operator accepts that nonce only while the create of the older version is unconfirmed (`status.createAttemptTime` is set), and the next create gets a random nonce.
+
+A provider that an older operator version created gets its record at the next reconcile, also while its object is being deleted. The record needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the provider ID with `ControlPlaneGone`: delete the provider in the dashboard.
 
 When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the provider in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the provider in the old deployment, then delete the ConfigMap.
 
@@ -160,7 +164,7 @@ A record without a provider ID is left by a create that the operator could not c
 3. When that provider is a leftover of the deleted object, delete it. The next sweep, within 10 minutes, then removes the record.
 4. When a person owns that provider, keep it and delete the ConfigMap.
 
-When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both.
+When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. Do not give them read access either: a record holds the secret nonce of a create. The install keeps namespace users out, but the built-in `view`, `edit` and `admin` roles include ConfigMaps: do not bind them to other users in that namespace, and do not bind them cluster-wide with a ClusterRoleBinding. If you install the operator in another namespace, change the namespace of both.
 
 ### Stuck finalizer
 

docs/how-to/manage-mcp-servers.md

@@ -95,7 +95,7 @@ The sample, with its Secret, is in `config/samples/coder_v1alpha1_codermcpserver
 
 - `controlPlaneRef`, `organization` and `slug` cannot change. The model sees the tools as `<slug>__<tool>`. To rename a slug, delete the object and create a new one. Chats then lose the old server.
 - The operator never takes over a config that it did not create. When the organization already has a config with the slug, the object stalls with `SlugConflict`, and the operator sends no write.
-- The operator creates the config disabled, with a temporary display name. It then applies the spec. Members do not see the config before that.
+- The operator creates the config disabled, with a temporary display name that holds a secret nonce (see [Cleanup records](#cleanup-records)). It then applies the spec. Members do not see the config before that.
 - The operator pins the organization ID at the first lookup. If the name later resolves to another ID, the object stalls with `OrganizationChanged`. Re-create the object to use the new organization.
 - Coder's `user_oidc` auth type is not supported. If someone sets it in the dashboard, the operator sets the auth type back at the next sync.
 
@@ -218,13 +218,17 @@ Delete `CoderMCPServer` objects before their control plane and before their name
 
 ### Cleanup records
 
-Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-<object UID>` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the config's temporary display name, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard.
+Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-<object UID>` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard.
+
+The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. The rules are the same as for [AI providers](manage-ai-providers.md#cleanup-records). Namespace users cannot read the record. Coder shows the nonce until the operator applies the spec. Until the operator knows the config ID, every create sends the same nonce. After it knows the ID, it takes no other config by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:<object UID>`. The operator accepts that nonce only while the create of the older version is unconfirmed (`status.createAttemptTime` is set), and the next create gets a random nonce. Do not give other users read or write access to ConfigMaps in the operator's namespace, also not through a ClusterRoleBinding of `view`, `edit` or `admin`.
+
+The record of an older config needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the slug and the config ID with `ControlPlaneGone`: delete the config in the dashboard.
 
 When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the config in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the config in the old deployment, then delete the ConfigMap.
 
 The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap.
 
-A record without a config ID is left by a create that the operator could not confirm. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the temporary display name, the sweep removes the record. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand:
+A record without a config ID is left by a create that the operator could not confirm. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the recorded nonce, the sweep removes the record. So when a person edits both the slug and the display name of the config of a lost create answer, nothing links that config to the record any more: the sweep removes the record, and the config stays in Coder. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand:
 
 1. Read the record and its Event:
 

internal/controller/cleanup_record.go

@@ -2,8 +2,10 @@ package controller
 
 import (
 	"context"
+	"crypto/rand"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/coder/coder/v2/codersdk"
@@ -22,8 +24,9 @@ import (
 // which Coder deployment holds the resource and how to find it. It stays when
 // the object goes without a confirmed delete in Coder (for example in a
 // namespace deletion), so a later sweep through a control plane of the same
-// deployment can delete the resource. A record holds no secrets. The
-// ConfigMap creation time is the record's created-at.
+// deployment can delete the resource. A record holds no credentials, but the
+// nonce of a provider or MCP server config is a secret until its create
+// commits (#426). The ConfigMap creation time is the record's created-at.
 const (
 	cleanupRecordPrefix = "ck8s-cleanup-"
 	// cleanupRecordLabel marks a record. Its value is the object kind.
@@ -53,7 +56,7 @@ type cleanupRecord struct {
 	// database, so it names the database that holds the resource.
 	DeploymentID string
 	// Nonce finds a resource whose Coder ID is not known yet: the display
-	// name of a provider or of an MCP server config.
+	// name that the last create of a provider or MCP server config sent.
 	Nonce string
 	// CoderID is the Coder ID of the resource once the operator knows it.
 	CoderID string
@@ -71,6 +74,62 @@ type cleanupRecord struct {
 
 func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) }
 
+// The display name of a create of a provider or MCP server config is prefix
+// and a random secret (#426). Only the record holds it, and namespace users
+// cannot read records, so nobody can give another resource that display name
+// before the create commits. Operator versions before #426 sent prefix and
+// the object UID, which anyone who can read the object sees. That nonce
+// proves only a create that is still pending, and the next create gets a
+// random one.
+
+// nextCreateNonce returns the nonce for a create of an object whose record
+// is previous. A create of the previous nonce can still commit while the
+// record has no Coder ID, and only that nonce then proves it, so it is sent
+// again. Once the Coder ID is known, Coder has shown the nonce as a display
+// name, so a new create gets a new one.
+func nextCreateNonce(prefix string, previous *cleanupRecord) string {
+	if previous != nil && previous.CoderID == "" && isRandomCreateNonce(prefix, previous.Nonce) {
+		return previous.Nonce
+	}
+	return prefix + rand.Text()
+}
+
+// keptCreateNonce returns the nonce for a record write without a create. An
+// object without a record is from before records existed, so a create of it
+// sent prefix and the object UID.
+func keptCreateNonce(prefix string, uid types.UID, previous *cleanupRecord) string {
+	if previous != nil {
+		return previous.Nonce
+	}
+	return prefix + string(uid)
+}
+
+// adoptionNonce returns the nonce that proves that a resource with the Coder
+// ID coderID is the object's: the recorded nonce while the record has no
+// Coder ID, or for the recorded ID itself. Once the operator knows the ID,
+// Coder has shown the nonce, so it proves nothing about another resource.
+// The nonce prefix and object UID of an older record prove only a create that
+// is still pending: anyone who can read the object knows them. Without a
+// record it returns "".
+func adoptionNonce(prefix string, record *cleanupRecord, coderID string, pending bool) string {
+	if record == nil || (record.CoderID != "" && record.CoderID != coderID) || (!pending && !isRandomCreateNonce(prefix, record.Nonce)) {
+		return ""
+	}
+	return record.Nonce
+}
+
+// validCreateNonce reports a nonce that the operator can have written in the
+// record of uid.
+func validCreateNonce(prefix string, uid types.UID, nonce string) bool {
+	return nonce == prefix+string(uid) || isRandomCreateNonce(prefix, nonce)
+}
+
+func isRandomCreateNonce(prefix, nonce string) bool {
+	secret, ok := strings.CutPrefix(nonce, prefix)
+	// rand.Text returns at least 26 characters of the base32 alphabet.
+	return ok && len(secret) >= 26 && strings.Trim(secret, "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567") == ""
+}
+
 // errNotACleanupRecord marks a ConfigMap of a record's name that is not a
 // valid record of that object. The operator never writes over it or deletes it.
 var errNotACleanupRecord = errors.New("not a cleanup record of this object")

internal/controller/cleanup_sweep.go

@@ -313,8 +313,8 @@ func (r *CleanupRecordReconciler) matchingControlPlanes(ctx context.Context, rec
 // then returns gone when a read by a site owner finds no provider of that
 // name and type, and kept otherwise.
 func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) {
-	if record.Nonce != aiProviderNoncePrefix+string(record.UID) {
-		return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce)
+	if !validCreateNonce(aiProviderNoncePrefix, record.UID, record.Nonce) {
+		return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has a nonce that the operator does not write", record.UID)
 	}
 	if step := requireSiteOwner(ctx, sdk); step != nil {
 		// Only a site owner can trust Coder's 404 for a provider.
@@ -397,8 +397,8 @@ func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *clean
 // it. It then returns gone when a list by a site owner shows no config with
 // the record's slug and none with its nonce, and kept otherwise.
 func sweepMCPServer(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) {
-	if record.Nonce != mcpServerNoncePrefix+string(record.UID) {
-		return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce)
+	if !validCreateNonce(mcpServerNoncePrefix, record.UID, record.Nonce) {
+		return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has a nonce that the operator does not write", record.UID)
 	}
 	orgID, err := uuid.Parse(record.Organization)
 	if err != nil || orgID == uuid.Nil || orgID.String() != record.Organization || record.Slug == "" {