diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index b1c024f82..9fee2b641 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -2,6 +2,7 @@ package controller import ( "context" + "errors" "fmt" "time" @@ -26,6 +27,32 @@ const ( aiProviderCreateSettle = 2 * time.Minute ) +// coderNotFoundMessage is the fixed message of Coder's resource 404, which +// the provider routes answer (coderd/httpapi/httpapi.go:149-155 and +// coderd/ai_providers.go:636-638 in v2.37.3). A 404 without it, for example +// an HTML page of a proxy or "Route not found." for a wrong base URL, does +// not prove that Coder has no such provider (#323). +const coderNotFoundMessage = "Resource not found or you do not have access to this resource" + +// isCoderResourceNotFound reports a 404 that Coder itself answered. +func isCoderResourceNotFound(err error) bool { + var sdkErr *codersdk.Error + return isCoderNotFound(err) && errors.As(err, &sdkErr) && sdkErr.Message == coderNotFoundMessage +} + +// checkAIProviderID fails unless status.providerID is a canonical UUID. The +// controller puts it into /api/v2/ai/providers/{id}, which also accepts a +// name, and the SDK resolves ".." segments, so any other value could send +// the operator token to another Coder route (#323). Only the controller +// writes it, so another value is impossible. +func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { + id, err := uuid.Parse(provider.Status.ProviderID) + if err != nil || id == uuid.Nil || id.String() != provider.Status.ProviderID { + return fmt.Errorf("assertion failed: AI provider %s/%s has status.providerID %q, which is not a canonical UUID", provider.Namespace, provider.Name, provider.Status.ProviderID) + } + return nil +} + // deleteStep handles an object that is being deleted. It deletes in Coder // only a provider that this object proved it created (plan S7), and it // returns release when the finalizer can go. Otherwise the step says why @@ -81,10 +108,14 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // only the nonce and the type prove that the provider is this object's. func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider) (*aiProviderStep, error) { id := provider.Status.ProviderID - if id == "" { + if id != "" { + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } else { found, err := sdk.AIProvider(ctx, provider.Name) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): // A 404 proves absence only for a site owner. A role change in // Coder has no watch, so the wait uses the delete poll. if step := requireSiteOwner(ctx, sdk); step != nil { @@ -115,7 +146,7 @@ func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *code } id = found.ID.String() } - if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderNotFound(err) { + if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) { return aiProviderWait(aiProviderDeletePoll, "CoderUnavailable", "delete provider %s (ID %s): %s", provider.Name, id, coderErrorSummary(err)), nil } return nil, nil diff --git a/internal/controller/coderaiprovider_delete_test.go b/internal/controller/coderaiprovider_delete_test.go index 598ad2d0f..3f70dd89b 100644 --- a/internal/controller/coderaiprovider_delete_test.go +++ b/internal/controller/coderaiprovider_delete_test.go @@ -326,3 +326,92 @@ func TestCoderAIProviderDeleteWithControlPlaneGoing(t *testing.T) { require.Equal(t, requests, env.fake.totalRequests(), "no Coder request") }) } + +// #323: status.providerID goes into /api/v2/ai/providers/{id}, which also +// takes a name, and the SDK resolves "..". A value that is not a canonical +// UUID fails an assertion before any Coder request, on the delete, sync and +// disable paths. +func TestCoderAIProviderForgedProviderID(t *testing.T) { + t.Parallel() + forged := map[string]string{ + "path traversal": "../../users/5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11", + "name": "openai", + "braces": "{5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11}", + "upper case": "5D0C6C2E-3A4D-4C55-9D2F-3F1B1F0C9A11", + "nil UUID": "00000000-0000-0000-0000-000000000000", + } + for _, path := range []string{"delete", "sync", "disable"} { + for name, id := range forged { + t.Run(path+"/"+name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + provider.Status.ProviderID = id + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + switch path { + case "delete": + require.NoError(t, k8sClient.Delete(ctx, provider)) + case "disable": + editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, "coder.com/credential") }) + } + requests := env.fake.totalRequests() + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "assertion failed: AI provider "+env.namespace+"/openai has status.providerID") + require.ErrorContains(t, err, "which is not a canonical UUID") + require.Equal(t, requests, env.fake.totalRequests(), "no Coder request") + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers) + }) + } + } +} + +// #323: a delete answered 404 without Coder's JSON error, for example an +// HTML page of a proxy, does not prove that the provider is gone. The +// finalizer stays and the delete is retried. +func TestCoderAIProviderDeleteNotFoundFromProxy(t *testing.T) { + t.Parallel() + for name, fault := range map[string]fakeFault{ + "HTML page": {Status: http.StatusNotFound, HTML: "404 Not Found"}, + "other message": {Status: http.StatusNotFound, Message: "Route not found."}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + env.fake.failNext(routeDeleteAIProvider, fault) + got, result := env.deleteAIProvider(ctx, t, provider) + require.NotNil(t, got) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers) + require.Equal(t, 60*time.Second, result.RequeueAfter) + _, found := env.fake.aiProvider("openai") + require.True(t, found) + + got, _ = env.reconcileDeleting(ctx, t, got) + require.Nil(t, got, "Coder's own answer releases the finalizer") + _, found = env.fake.aiProvider("openai") + require.False(t, found) + }) + } +} + +// #323: with only a create marker, a lookup 404 that Coder did not answer +// proves nothing either. +func TestCoderAIProviderDeletePendingCreateNotFoundFromProxy(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + provider.Finalizers = []string{coderv1alpha1.CoderAIProviderCleanupFinalizer} + require.NoError(t, k8sClient.Update(ctx, provider)) + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-3 * time.Minute)} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound, HTML: "404 Not Found"}) + got, _ := env.deleteAIProvider(ctx, t, provider) + require.NotNil(t, got) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers) + got, _ = env.reconcileDeleting(ctx, t, got) + require.Nil(t, got, "Coder's own 404 releases the finalizer") +} diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index 8e305ec8d..1c3eb361d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -33,6 +33,9 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if provider.Status.ProviderID == "" || credErr == nil { return nil, fmt.Errorf("assertion failed: AI provider %s/%s needs a provider ID and a credential failure to be disabled", provider.Namespace, provider.Name) } + if err := checkAIProviderID(provider); err != nil { + return nil, err + } // Status messages are cut at 256 characters, so the new fact goes first. // No Kubernetes event follows a dashboard enable, so the stall resyncs. stalled := func(format string, args ...any) *aiProviderStep { diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go index 3a79b26e4..ed6734f1e 100644 --- a/internal/controller/coderaiprovider_sync.go +++ b/internal/controller/coderaiprovider_sync.go @@ -34,6 +34,9 @@ func (r *CoderAIProviderReconciler) syncAIProvider( if cred == nil { return nil, false, fmt.Errorf("assertion failed: AI provider %s/%s has a provider ID without a credential", provider.Namespace, provider.Name) } + if err := checkAIProviderID(provider); err != nil { + return nil, false, err + } applied := provider.Status.AppliedGeneration == provider.Generation found, err := sdk.AIProvider(ctx, provider.Status.ProviderID) switch {